-
Notifications
You must be signed in to change notification settings - Fork 25
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
[konflux] add assembly type to app name, component name #1105
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
app_name = self._config.group_name.replace(".", "-") | ||
group_name = self._config.group_name.replace(".", "-") | ||
app_name = group_name | ||
app_name = f"{app_name}-{metadata.runtime.assembly}" |
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.
use group_name directly
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.
I'm using the group name below: https://github.com/openshift-eng/art-tools/pull/1105/files#diff-e45d70130344e3d45bd59cb091bd0a794121c675ce90f17c5b4e4702ded620ffR183
So didn't want to replace the .
with -
again
component_name = f"{app_name}-{metadata.distgit_key}".replace(".", "-").replace("_", "-") | ||
# Since Component is a CRD, it has to be unique in the namespace. Also once a Component and Application is created, the branches that its linked to cannot be modified | ||
# Hence prefixing the assembly and version to the component name to make it unique, eg: "stream-4-18-ose-etcd" | ||
component_name = f"{metadata.runtime.assembly}-{group_name.removeprefix(f'openshift-')}-{metadata.distgit_key}".replace(".", "-").replace("_", "-") |
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.
rather than computing something from distgit-key, can we have the konflux name entity hard coded rather than calculated through an attribute in ocp-build-data?
7635ead
to
50c127b
Compare
@ashwindasr: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
We might need to ship a release coming from mixed assemblies (e.g. stream and 4.y.z). I am not sure if it will be an issue if we separate assemblies and components by assembly name. Do you have any other reason (except shortening resource names) to make this change? Also it guess it might be fine to just truncate the distgit keys in some way or having a new field as suggested by @joepvd . What do you think? |
Yeah this change makes me weary a little bit.. we have to do a good amount of config admin work with application names / component names in konflux-release-dev repo for releases. So why exactly are we doing this? or what is the problem right now that this would solve? |
When we tried to trigger
stream
builds, openshift did not let us update theComponent
since it was tied to atest
GH branch. Hence we want to separateApplication
andComponent
CRDs based on version and assembly and names have to be unique in a namespace.So an application from now on will look like
openshift-4-18-stream
/openshift-4-18-test
and a component will look likestream-4-18-ose-etcd
/test-4-18-ose-etcd
Also since Kubernetes resources have a maximum character length of 63 characters, lets no onboard a component with very long distgit names.