-
Notifications
You must be signed in to change notification settings - Fork 312
Included qualifier value in bundle and build manifests #5634
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
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
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.
Hi @ajleong623
Can you also please add tests for changed functionality?
Looks like current tests are failing for formatting as well.
Adding @rishabh6788 @peterzhuamazon for additional reviews.
if semver.compare(self.manifest.build.version + (("-" + self.manifest.build.qualifier) | ||
if self.manifest.build.qualifier else None), '2.12.0') != -1: |
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 dont believe qualifier provides any value here. @rishabh6788 please correct me if I am wrong
cmd = f"bash {script} -b {single_node.endpoint} -p {single_node.port} -s {str(security).lower()} -t {self.component.name} -v {self.bundle_manifest.build.version + (("-" + self.bundle_manifest.build.qualifier) | ||
if self.bundle_manifest.build.qualifier else None)} -o default -r false" |
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.
@peterzhuamazon Do we also need to make changes to defualt scripts to accommodate qualifier?
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
Is there a way to have the workflows automatically run for subsequent pushes? |
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
@gaiksaya I added a tests for the assemble workflow, and I think the build workflow should be covered with the qualifiers in |
You can download one for from http://ci.opensearch.org/ci/dbc/distribution-build-opensearch/3.0.0-alpha1/10932/linux/x64/tar/builds/opensearch/manifest.yml or generate locally using 3.0.0-alpha1/beta1 manifest. |
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
…for now Signed-off-by: Anthony Leong <[email protected]>
@gaiksaya I have added the tests for the assemble and build workflows, however, I will have to look back at how the test_workflow and release_notes workflow will be affected and make those tests (original changes). Would it be possible to do that in another issue? Otherwise, I think all I have to do is make sure the test workflows and linting pass. Could I have the workflows be run when I push since it seems like running the workflows has to be approved by a maintainer, currently? |
Signed-off-by: Anthony Leong <[email protected]>
@gaiksaya This is not related to the PR, but congrats on spearheading the release of version 3.2.0! Also, I cannot wait to see your talk at OpenSearchCon. |
Will monitor and approve. This change might be huge and might need some changes on metrics side as well. We can go in steps. Will also need to exercise this a bit so that releases are not affected |
@gaiksaya yes. I noticed that there are a lot of components affected, but I wanted to break it up into steps. At the moment, I have included the main workflows of building and assembling, but there might be other components that are affected to a lesser extent. What is the metrics side, and is there a process to exercising the change more to not affect releases? |
We index all the data in opensearch metrics cluster at metrics.opensearch.org. Apart from that we have various CI libraries that use manifest at it core to carry out multiple activities https://github.com/search?q=repo%3Aopensearch-project%2Fopensearch-build-libraries%20qualifier&type=code We can take it step by step and get changes sooner to exercise and fix the issues. |
@gaiksaya which next steps are you thinking about? For now, I would like to pass the workflows. It looks like changes also have to be made to the CI libraries? |
Signed-off-by: Anthony Leong <[email protected]>
Description
Currently, the input manifest schema has a qualifier attribute which is not included in the bundle and build manifests. Now whenever the versions of the build and bundle manifest are used in the workflow, the qualifier is also appended to the version of the build or bundle manifests for those cases. Additionally, the schema for the build and bundle manifests will keep track of a qualifier value.
Issues Resolved
#5386
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.