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

Fix registry-proxies flag #70

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Fix registry-proxies flag #70

merged 1 commit into from
Mar 11, 2024

Conversation

miguelbernadi
Copy link
Contributor

Change-Id: Ie82c2558bc48974d1b0868b375f4c4dedb4acab3

@joaoqalves joaoqalves self-requested a review January 18, 2024 11:17
@joaoqalves
Copy link
Contributor

@miguelbernadi Is there a way to have a test and avoid this from happening for other flags?

@miguelbernadi
Copy link
Contributor Author

I can't think of a way to do that off the top of my head. We'd need to test it externally (by supplying a value to the flag) and verify the internal variable gets modified. This is quite close to testing the flag package directly in my POV.

This should be flagged by the compiler as a non-utilised variable. It's not, because we initialize the variable, assign it a value and then pass it forward. So if it is not used it stiln has the default value. If we changed to initialise and assign the variable in the same step, this kind of mistake would be a compiler error. I'm not clear on the downsides of that approach as I can't spot any right away. I'm referring to using flag.String instead of flag.StringVar.

Change-Id: Ie82c2558bc48974d1b0868b375f4c4dedb4acab3
Copy link
Contributor

@InsomniaCoder InsomniaCoder left a comment

Choose a reason for hiding this comment

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

I'm a bit lost on what was problem caused by this wrong flag. It'd be nice to have some reference 🙏🏼

@tjamet tjamet added this pull request to the merge queue Mar 11, 2024
@tjamet tjamet removed this pull request from the merge queue due to a manual request Mar 11, 2024
@tjamet tjamet enabled auto-merge March 11, 2024 10:29
@tjamet tjamet added this pull request to the merge queue Mar 11, 2024
Merged via the queue into adevinta:main with commit c0e49e0 Mar 11, 2024
1 check passed
@miguelbernadi
Copy link
Contributor Author

I'm a bit lost on what was problem caused by this wrong flag. It'd be nice to have some reference 🙏🏼

When I found the issue, there was no problem, as we were not using the flag, but it was obviously incorrect, so the fix had no description.

Right now, the problem would be that there is a will to use this flag, which was only possible after.

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