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

Support release iteration value for assemble workflow rpm #1921

Conversation

peterzhuamazon
Copy link
Member

@peterzhuamazon peterzhuamazon commented Apr 7, 2022

Signed-off-by: Peter Zhu [email protected]

Description

Support release iteration value for assemble workflow rpm.

This PR add support for release/iteration value in rpm/deb packages.

It allows us to build packages with the same version without overriding existing packages.
It also allow users to upgrade the package to the same version but latest release/iteration.

-rw-r--r-- 1 <> 390033760 Apr  7 02:09 opensearch-1.3.0-1-linux-x64.rpm
-rw-r--r-- 1 <> 390033852 Apr  7 01:59 opensearch-1.3.0-2-linux-x64.rpm
-rw-r--r-- 1 <> 386532844 Apr  7 02:04 opensearch-1.3.1-1-linux-x64.rpm
opensearch-1.3.0-1.x86_64.rpm
Name        : opensearch
Version     : 1.3.0
Release     : 1
Architecture: x86_64

opensearch-1.3.0-2.x86_64.rpm
Name        : opensearch
Version     : 1.3.0
Release     : 2
Architecture: x86_64

It also allows us to assign the $BUILD_NUMBER to release/iteration in Jenkins so we can separate then when pushing to staging yum repo for testing.

Thanks.

Issues Resolved

#1545

Check List

  • Commits are signed per the DCO using --signoff

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.

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #1921 (483bdcb) into main (4df94c4) will increase coverage by 0.39%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1921      +/-   ##
============================================
+ Coverage     94.60%   95.00%   +0.39%     
  Complexity       20       20              
============================================
  Files           178       44     -134     
  Lines          3633      220    -3413     
  Branches         27       27              
============================================
- Hits           3437      209    -3228     
+ Misses          192        7     -185     
  Partials          4        4              
Impacted Files Coverage Δ
src/assemble_workflow/assemble_args.py
src/assemble_workflow/bundle.py
src/assemble_workflow/bundle_recorder.py
src/assemble_workflow/bundle_rpm.py
src/assemble_workflow/dist.py
src/run_assemble.py
src/test_workflow/bwc_test/bwc_test_runners.py
...est_workflow/test_result/test_component_results.py
src/ci_workflow/ci_check_list_source_ref.py
src/assemble_workflow/bundle_locations.py
... and 124 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4df94c4...483bdcb. Read the comment docs.

@peterzhuamazon
Copy link
Member Author

peterzhuamazon commented Apr 7, 2022

Hi @spotrh hope you can help us clear some confusions.

Let' s say we build rpm for $BUILD_NUMBER=123 in Jenkins.
If we assign 123 to release then it will show:

opensearch-1.3.0-123.x86_64.rpm
Name        : opensearch
Version     : 1.3.0
Release     : 123
Architecture: x86_64

If we release this directly as the 1st 1.3.0 release, would this be a problem after all?
To my understanding there is no technical issues.
Just the 1st release of 1.3.0 being 123 release is a bit weird.

Thanks.

@peterzhuamazon
Copy link
Member Author

Hi @dblock @tianleh,

Please review this I will add test cases later.

Thanks.

dest="release_iteration",
type=check_positive,
default="1",
help="The release/iteration number of deb/rpm packages, allow multiple revision of same package version (e.g. 2.0.0-1, 2.0.0-2, 2.0.0-3)"
Copy link
Member

Choose a reason for hiding this comment

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

revision -> revisions

class AssembleArgs:
manifest: IO
keep: bool

def __init__(self) -> None:

def check_positive(value) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

You can add type for the input. value: str

Copy link
Member Author

Choose a reason for hiding this comment

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

This does not allow me to block non-positive inputs.

@spotrh
Copy link

spotrh commented Apr 7, 2022

Hi @spotrh hope you can help us clear some confusions.

Let' s say we build rpm for $BUILD_NUMBER=123 in Jenkins. If we assign 123 to release then it will show:

opensearch-1.3.0-123.x86_64.rpm
Name        : opensearch
Version     : 1.3.0
Release     : 123
Architecture: x86_64

If we release this directly as the 1st 1.3.0 release, would this be a problem after all? To my understanding there is no technical issues. Just the 1st release of 1.3.0 being 123 release is a bit weird.

The "Release" field is intended to be incremented when changes are made to the package that do not involve updating to a newer version of the software. When Version increments, you should reset Release (to prevent it from growing absurdly large)

I'm not sure why you'd want to set Release to "123" as a starting value, but it's not harmful.

@peterzhuamazon
Copy link
Member Author

peterzhuamazon commented Apr 7, 2022

Hi @spotrh hope you can help us clear some confusions.
Let' s say we build rpm for $BUILD_NUMBER=123 in Jenkins. If we assign 123 to release then it will show:

opensearch-1.3.0-123.x86_64.rpm
Name        : opensearch
Version     : 1.3.0
Release     : 123
Architecture: x86_64

If we release this directly as the 1st 1.3.0 release, would this be a problem after all? To my understanding there is no technical issues. Just the 1st release of 1.3.0 being 123 release is a bit weird.

The "Release" field is intended to be incremented when changes are made to the package that do not involve updating to a newer version of the software. When Version increments, you should reset Release (to prevent it from growing absurdly large)

I'm not sure why you'd want to set Release to "123" as a starting value, but it's not harmful.

I think I misunderstand some concept of my upgrade design.
I will still allow this change to go in but will use release=1 for each build.

Thanks.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The fact that this is a new parameter that is only specific to deb/rpm, but is passed into a generic workflow is a red flag to me. Why do we need this at all? We don't vary build numbers for .tar.gz, so why are we trying to vary build numbers for deb?

  • Upgrading between builds is not a supported scenario.
  • We distinguish between builds by placing output in different build folders (with job number) via Jenkins, so why do we want to do this for rpm?

If you want to vary the build number, we should do it for all artifacts. Every build would generate a patch number based on, for example, date and time, e.g. 2.0.0.4071558 (April 7, 3:58).

class AssembleArgs:
manifest: IO
keep: bool

def __init__(self) -> None:

def check_positive(value) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

Make this method private.

dest="release_iteration",
type=check_positive,
default="1",
help="The release/iteration number of deb/rpm packages, allow multiple revision of same package version (e.g. 2.0.0-1, 2.0.0-2, 2.0.0-3)"
Copy link
Member

Choose a reason for hiding this comment

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

Help missing period, is too long and has grammar issues. Just say "Release number."

@peterzhuamazon
Copy link
Member Author

After some review I think this PR is not needed anymore.
I have figured out a better way to test upgrade across different builds using latest.

Thanks.

@peterzhuamazon peterzhuamazon deleted the opensearch-assemble-rpm-release branch May 16, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants