-
Notifications
You must be signed in to change notification settings - Fork 20
Rename server_url to server_hostname in registrations #399
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
base: main
Are you sure you want to change the base?
Conversation
| "organization": "replace-with_org", | ||
| "server_url": "replace-with_server_url", | ||
| "base_url": "replace_with-base_url", | ||
| "server_hostname": "subscription.rhsm.redhat.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI we would have to change "images" here, no? This is just a direct mapping of https://github.com/osbuild/images/blob/423d32a0853f1f7ea6ab03507090a11903792dfe/pkg/customizations/subscription/subscription.go#L11 which is also used in composer. Don't get me wrong, I'm in favor for making this cleaner maybe we could just with alias fields or something so there is less risk of breaking anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is why I am asking for opinions on how to approach this.
From what I remember, @achilleas-k was always for just "break it, fix it quickly".
|
This is just a README change? The format of the config needs to be changed in images. FWIW, the actual underlying command has already been updated internally: osbuild/images#2071 The question is where and how this was exposed and what should we change (and how)? I think (off the top of my head now) that we never exposed these options in composer-cli for osbuild-composer. They were only ever set by the service, so as far as that's concerned, there is little issue. If we change the option name in images and then osbuild-composer, we would have to coordinate the change with the frontend when the backend gets updated, but that's simple enough.
The external option mirroring the internal one is convenient. We don't know if this option has ever been used, but we should at least try to maintain some backwards compatibility. I'd at least deprecate the old option name with a warning for a version or two before removing it. As far as I know, even back when it was called |
|
Also, keep in mind, this was only ever set in the service to register with the staging subscription management service when building images in stage.c.rh.c. It might make more sense to drop it altogether. |
|
Actually, @jlsherrill just opened a PR that converts the field from
This was meant to start the discussion and either do a code change, or whatever is needed. |
I just noticed that we have a very misleading field name in registrations:
server_url. This must not be URL, in fact, it breaks RHSM client it only accepts a hostname or IP:Base URL is correct.
I would like to fix this there is still time, tho, @achilleas-k already started using this in his monster CLI integration PR in composer. This draft does not anything yet, I want to discuss this how to approach this.
There are several options: