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

[core-service] Rename enable_http to allow_http_base_urls #1061

Merged
merged 5 commits into from
Aug 8, 2024

Conversation

brandoncorrea
Copy link
Contributor

@brandoncorrea brandoncorrea commented Jul 30, 2024

  • Rename all usages of EnableHTTP to AllowHTTPBaseUrls
  • Add command-line flag, allow_http_base_urls
  • Deprecate command-line flag, enable_http, in favor of allow_http_base_urls

Clients of core-service will need to update the command-line parameter, enable_http to allow_http_base_urls, namely Monitoring – core_service.sh L21/33.

Created for Issue #788

Copy link

linux-foundation-easycla bot commented Jul 30, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@brandoncorrea brandoncorrea marked this pull request as draft July 30, 2024 13:16
@brandoncorrea brandoncorrea marked this pull request as ready for review August 2, 2024 03:55
Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

Thank you @brandoncorrea for your contribution.
In case the old flag is provided, core-service will fail with an unrecognized flag error.
Since this flag should only be used for development purpose, I think it should be ok as-is. However, the flag is used in the context of the monitoring repository tests and we will need to coordinate this change.
Therefore, we will need to wait a bit before merging this PR. An option to not wait would be to keep backward compatibility and keep -enable_http possible with a deprecation warning, migrate the monitoring repository and clean it up after.
I think keeping backward compatibility would be great if you have time for that.

@brandoncorrea
Copy link
Contributor Author

I'm in favor of supporting backwards compatibility. I'll update the PR accordingly.

@brandoncorrea
Copy link
Contributor Author

@barroco Both enable_http and allow_http_base_urls flags are now supported.

Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

Thank you very much for the change! This is looking great. A last detail would be to print a warning if the old flag is used as well as update the description of the usage description of the old flag indicating it is deprecated in favour of the new one.

@brandoncorrea
Copy link
Contributor Author

A warning has been added when the old flag is set, preferring the new flag if both are set.

Looking further into how flags work, I realized my first backwards-compatibility code wasn't technically correct because flags aren't initialized at the head of the file (everything is default). I moved this to main where logging and flags have already been initialized.

@barroco
Copy link
Contributor

barroco commented Aug 8, 2024

Thanks @brandoncorrea for your efforts. Last thing, could you please update the PR description to reflect the latest changes ?

@brandoncorrea
Copy link
Contributor Author

@barroco description has been updated!

@barroco barroco merged commit b6ad9f9 into interuss:master Aug 8, 2024
6 checks passed
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.

2 participants