Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework PR/MR feature #219

Merged
merged 21 commits into from
Oct 8, 2021
Merged

Conversation

neomilium
Copy link
Contributor

This one is bigger than previous ones... even I tried to fine split.

I worked in TDD, so I started with tests, then made some progress, saw that my work will affect more than expected, wrote more tests, continue with a light refactoring and finally rework all PR/MR as a generic GitService.

The hardest part on this topic is to "implement" all unspecified behavior, so I tried to extend the test coverage.
I tried to keep the backward compatibility, but some specific corner cases could fail.

At this point, I made my best to prevent from breaking compatibility but I think some fundamentals should be re-written to make sense.

Nevertheless this rewrite allow us to easily fix some issues later, and provide a generic way to use "git service", which can be reuse for other features (e.g. #158).

@neomilium neomilium force-pushed the rework-prmr-feature branch from 24d4e68 to c384448 Compare April 23, 2021 15:03
@bastelfreak bastelfreak requested review from raphink and ekohl and removed request for raphink April 26, 2021 20:24
Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but it doesn't hurt if someone reviews this with more Ruby knowledge. @ekohl @raphink can one of you take a look as well?

bittner and others added 16 commits October 5, 2021 22:24
This commit starts the refactoring of PR/MR feature code.

It provides a more readable code by:
- renaming method #manage for GitHub and GitLab class to open_pull_request
- extracting the credentials and endpoints to use in only one method: GitService#instantiate

It also provides a more understandable messages:
- `msync` does not warn anymore if no environmment variables are sets, but correctly configured in module options
- when no tokens are provided, the message now only contains the service that requires to specify token

Note: This message could be improved to help the user to setup its
modules configuration or ask him to set the environment variable.
This commit renames ModuleSync::PR module to ModuleSync::GitService.

Git service classes (ie. GitHub and GitLab) can now be used to implement
more features than PR/MR opening. (e.g voxpupuli#158)
This commit only changes notation to ease code review, it does not
change any code.
This commit only changes notation to ease further context introduction, it does not
change any code.
This commit adds a check to ensure no PR/MR are made with the same source
and target branch.

This commit implements a facade class (ie. GitService::Base) to prevent
adding duplicate code in the git services and remove any global options[] usage.

It lets SourceCode#open_pull_request to craft the right parameters to
pass to the GitService once, so it deduplicates the code in specialised
service.

By design, this allows to honor easily the default repo branch.
Thanks to previous commit, this commit fixes voxpupuli#207 and now honors the
default repo branch.
This commit removes the outputs like "Would submit PR" when there are no
changes after update (in no-op mode).
At this point, GitService should guess "git service" type (ie. GitHub or
GitLab) and rely on multiple sources to do.

As there is no more default (ie. which was GitHub) and the local git
repo used does not provide guessable service type, we need to add this
`base_url` options on each tests before
This commit implements a method to know which Git service (ie. GitHub or
GitLab) is used to host source code.

 * introduces a specific error when creadentials are missing
 * adds method to guess git service type based on a SourceCode instance
 * adds method to find with git service's endpoint should be used
 * adds method to find token
 * adds a method to pack a configuration hash (type, endpoint, token)
   based on a SourceCode instance
This commit adds support for more repository notations and provides unit
tests.
@neomilium neomilium force-pushed the rework-prmr-feature branch from c384448 to ce795d5 Compare October 5, 2021 20:44
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #219 (e02e6a6) into master (f173ac3) will decrease coverage by 46.32%.
The diff coverage is 19.87%.

❗ Current head e02e6a6 differs from pull request most recent head 57ebbb4. Consider uploading reports for the commit 57ebbb4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #219       +/-   ##
==========================================
- Coverage   52.35%   6.02%   -46.33%     
==========================================
  Files          14      17        +3     
  Lines         510     846      +336     
==========================================
- Hits          267      51      -216     
- Misses        243     795      +552     
Impacted Files Coverage Δ
lib/modulesync.rb 0.00% <0.00%> (-37.41%) ⬇️
lib/modulesync/cli.rb 0.00% <0.00%> (-80.71%) ⬇️
lib/modulesync/git_service.rb 0.00% <0.00%> (ø)
lib/modulesync/git_service/factory.rb 0.00% <0.00%> (ø)
lib/modulesync/repository.rb 0.00% <0.00%> (-17.35%) ⬇️
lib/modulesync/source_code.rb 0.00% <0.00%> (-64.52%) ⬇️
lib/modulesync/git_service/base.rb 80.00% <80.00%> (ø)
lib/modulesync/git_service/github.rb 92.00% <100.00%> (ø)
lib/modulesync/git_service/gitlab.rb 90.90% <100.00%> (ø)
lib/modulesync/settings.rb 0.00% <0.00%> (-100.00%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f173ac3...57ebbb4. Read the comment docs.

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of stuff, not easy for a review, but overall LGTM!

@neomilium
Copy link
Contributor Author

I spotted a bug, and will fix it tomorrow.

@neomilium neomilium marked this pull request as draft October 5, 2021 21:29
@neomilium neomilium marked this pull request as ready for review October 5, 2021 23:00
@neomilium
Copy link
Contributor Author

@smortex @bastelfreak @bittner : Its ready to review!

@neomilium
Copy link
Contributor Author

@ekohl @raphink : Could you check this PR ?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look through the code and in general it looks good to me. It's pretty large so I may have missed things.

lib/modulesync/git_service.rb Outdated Show resolved Hide resolved
lib/modulesync/pr/github.rb Outdated Show resolved Hide resolved
spec/unit/modulesync/pr/github_spec.rb Outdated Show resolved Hide resolved
lib/modulesync/git_service/base.rb Show resolved Hide resolved
features/update/pull_request.feature Outdated Show resolved Hide resolved
@neomilium
Copy link
Contributor Author

@ekohl Codecov report is not up-to-date: I just removed a commit that adds cucumber to tests because it disturb the code coverage. I opened an issue here #236.

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing

lib/modulesync/git_service/factory.rb Outdated Show resolved Hide resolved
@neomilium neomilium force-pushed the rework-prmr-feature branch from 3abdb32 to 57ebbb4 Compare October 6, 2021 20:03
@neomilium
Copy link
Contributor Author

@ekohl @raphink Ready to merge?

@bastelfreak
Copy link
Member

I think it's good enough to be merged now

@bastelfreak bastelfreak merged commit 98fe295 into voxpupuli:master Oct 8, 2021
@neomilium
Copy link
Contributor Author

👍

@neomilium neomilium deleted the rework-prmr-feature branch October 8, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants