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

Remove redundant code and fix flag option #734

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

bmeneg
Copy link
Collaborator

@bmeneg bmeneg commented Aug 24, 2021

This MR has three different and minor fixes:

  1. Remove redundant code in internal/gitlab/gitlab.go::FindProject()
  2. The --project value was being used as the value for token variable instead of --token value.

Check the individual commits for more information.

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #734 (1326331) into master (2fb6734) will increase coverage by 0.09%.
The diff coverage is 45.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   54.71%   54.81%   +0.09%     
==========================================
  Files          77       77              
  Lines        5596     5610      +14     
==========================================
+ Hits         3062     3075      +13     
- Misses       2251     2253       +2     
+ Partials      283      282       -1     
Impacted Files Coverage Δ
cmd/ci_run.go 64.70% <0.00%> (ø)
cmd/mr_approve.go 70.27% <33.33%> (-1.61%) ⬇️
cmd/mr_unapprove.go 75.00% <47.36%> (+3.12%) ⬆️
cmd/note_common.go 54.57% <66.66%> (-0.24%) ⬇️
internal/gitlab/gitlab.go 7.27% <100.00%> (+0.20%) ⬆️

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 fd3d2ce...1326331. Read the comment docs.

Copy link
Owner

@zaquestion zaquestion left a comment

Choose a reason for hiding this comment

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

I think we don't actually need to deprecate the --project flag see comments and getCIRunOptions

cmd/ci_run.go Show resolved Hide resolved
cmd/ci_run.go Outdated Show resolved Hide resolved
@zaquestion
Copy link
Owner

Ah I'm realizing that marking for deprecation doesn't disable the functionality, tho I still have some concerns about removing it down the line. Despite the inconsistency in the tool, very fair assessment there, the --project flag serves a real purpose and powers a real workflow, which is consistent with the goals of the project ( as in it's not there just for api completeness, it's there because it's useful for doing a particular thing). At the least I'd suggest we open an issue to discuss how to support this in a way more consistent with the repo. I would also check what the lab ci status command does as I believe I added a way to specify project for that as part of #240

Copy link
Owner

@zaquestion zaquestion left a comment

Choose a reason for hiding this comment

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

downgrading my request for changes to a request for comment

@bmeneg
Copy link
Collaborator Author

bmeneg commented Aug 25, 2021

@zaquestion I replied to your specific comments in the code before realizing you did a general comment in the PR.
My comments to your comments still applies though :)

@bmeneg
Copy link
Collaborator Author

bmeneg commented Aug 25, 2021

ahh.. obviously I missed the lab ci status. Will check that as well and, possibly, open an issue to discuss it or to inform a possible conclusion (if we get to one here 😄)

@prarit
Copy link
Collaborator

prarit commented Sep 13, 2021

@bmeneguele , this PR is still in draft after 20 days. Is that intentional? I've been holding off reviewing until it was "Ready". Maybe that was a mistake on my part?

@bmeneg
Copy link
Collaborator Author

bmeneg commented Sep 13, 2021

@prarit no, it's intentional. I need to come back to this code and rework. But unfortunately I had no time to rethink it yet.

In lab.FindProject() the GetProject API is being used in the exact same way the
function lab.GetProject() is using. This patch removes this redundant code
and use the lab.GetProject() function instead.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
@bmeneg
Copy link
Collaborator Author

bmeneg commented Oct 1, 2021

To move this PR forward I'm going to remove the "deprecation" commit and move it to the next PR I'm preparing with the --follow logic.

The `token` variable was getting the `--project` option value instead of the
`--token` itself, making it completely unusable.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
@bmeneg bmeneg marked this pull request as ready for review October 1, 2021 17:47
@bmeneg bmeneg requested a review from zaquestion October 1, 2021 17:47
@bmeneg bmeneg changed the title Deprecates CI option and remove redundant code Remove redundant code and fix flag option Oct 1, 2021
@bmeneg
Copy link
Collaborator Author

bmeneg commented Oct 1, 2021

@zampierilucas @prarit @zaquestion please, feel free to re-review this patch, like I said in the previous comment, the part that generated the whole discussion was removed. I'll do that in the next PR. With that, only two really simple commits were left.

@bmeneg
Copy link
Collaborator Author

bmeneg commented Oct 4, 2021

downgrading my request for changes to a request for comment

Considering the above comment from @zaquestion and also the fact I'm going to open a new PR with the changes we discussed here so far, I'm going ahead and merge this PR.

@bmeneg bmeneg dismissed zaquestion’s stale review October 4, 2021 15:23

A new PR will be created with a more contextualized change set.

@bmeneg bmeneg merged commit ef2c893 into zaquestion:master Oct 4, 2021
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.

4 participants