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

Automate the creation of release tags #1666

Merged
merged 14 commits into from
Mar 7, 2022

Conversation

zelinh
Copy link
Member

@zelinh zelinh commented Feb 23, 2022

Signed-off-by: Zelin Hao [email protected]

Description

Create the release tag for plugins from the distribution-build manifest based on the version and build_id.
This workflow will fetch the commit_id from the manifest for each plugin and create the GitHub tag based on that, which could avoid discrepancies of using different commit id from release.

Issues Resolved

#378

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.

@zelinh zelinh requested a review from a team as a code owner February 23, 2022 00:32
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022

Codecov Report

Merging #1666 (d85d0fe) into main (986736d) will increase coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1666      +/-   ##
============================================
+ Coverage     94.62%   94.76%   +0.14%     
- Complexity       14       17       +3     
============================================
  Files           164      169       +5     
  Lines          3459     3518      +59     
  Branches         21       26       +5     
============================================
+ Hits           3273     3334      +61     
+ Misses          183      181       -2     
  Partials          3        3              
Impacted Files Coverage Δ
tests/jenkins/jobs/BuildManifest_Jenkinsfile 100.00% <ø> (ø)
src/jenkins/BuildManifest.groovy 92.68% <100.00%> (+1.01%) ⬆️
tests/jenkins/jobs/CreateReleaseTag_Jenkinsfile 100.00% <100.00%> (ø)
src/manifests/bundle_manifest.py 97.43% <0.00%> (-2.57%) ⬇️
src/assemble_workflow/dist.py 95.08% <0.00%> (-0.58%) ⬇️
src/manifests/build_manifest.py 100.00% <0.00%> (ø)
src/assemble_workflow/bundle_recorder.py 100.00% <0.00%> (ø)
deployment/lib/artifacts-public-access.ts 100.00% <0.00%> (ø)
src/assemble_workflow/dists.py 100.00% <0.00%> (ø)
deployment/lambdas/cf-url-rewriter/https-get.ts 100.00% <0.00%> (ø)
... and 4 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 986736d...d85d0fe. Read the comment docs.

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Please add tests for both groovy and JenkinsFile

echo "Creating $version release tag for $componetsNumber components in ths manifest"

withCredentials([usernamePassword(credentialsId: "${GITHUB_BOT_TOKEN_NAME}", usernameVariable: 'GITHUB_USER', passwordVariable: 'GITHUB_TOKEN')]) {
for (component in componentsName) {
Copy link
Member

Choose a reason for hiding this comment

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

I think its better to add try catch and check if the tag already exists. Some components cut the tag beforehand.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed and probably can also check whether the existing tag has the correct commitid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check below: If certain tag is existing in the current repo, we will just simply delete it and replace with the new one with the commit ID from the manifest.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should delete or skip it. @dblock any thoughts on this?

Copy link
Member

@dblock dblock Feb 28, 2022

Choose a reason for hiding this comment

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

I would skip it with a note if it's identical (someone did the right thing and manually created the tag, it's ok), and fail if the existing tag doesn't match (someone created the wrong tag, let's find out loudly).

Copy link
Member

Choose a reason for hiding this comment

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

@zelinh Can you work with the team to come up with an effort estimate to move the shell code with groovy? I would prefer to go that route if its a quick effort.

Copy link
Member

@dblock dblock Mar 3, 2022

Choose a reason for hiding this comment

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

I dislike the implementation in shell a lot, but with the rule of thumb that this is better than what we had (manual process) I vote to merge this as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the implementation here and remove shell block as much as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I would skip it with a note if it's identical (someone did the right thing and manually created the tag, it's ok), and fail if the existing tag doesn't match (someone created the wrong tag, let's find out loudly).

Hi @dblock What do you think about hard fail at the end rather than failing the entire workflow before all components are parsed?
Right now this just fails the workflow and until that component fixes the tag we cannot re-run. How about go and finish the workflow and hard fail at the end if there is a mismatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

More elaboration on the current status: when some component already cut the tag with different commit from the commit in the distribution manifest, the current workflow will fail immediately and ask user to fix it until it matches.

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Not too many issues with the PR but where are the tests?
Could you put this to draft PR and add test cases?
Thanks.

@zelinh zelinh marked this pull request as draft February 23, 2022 20:04
@zelinh zelinh marked this pull request as ready for review February 26, 2022 02:27
@bbarani bbarani requested a review from dblock February 28, 2022 18:42
@zelinh zelinh self-assigned this Feb 28, 2022
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
}
}
parameters {
string(
Copy link
Member

Choose a reason for hiding this comment

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

As an idea, this job could take a version number without a build ID, because the URL for the manifest for a well-known version should be known. Or it could take a URL to the manifest. This avoids the need to download anything from S3. It's no big deal though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting the manifest URL will still need the build ID, otherwise we will need to take the manifest URL as the parameter input. I think essentially both way are the same, good for either way.

Copy link
Member

@gaiksaya gaiksaya Mar 3, 2022

Choose a reason for hiding this comment

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

Also, We are bundling this manifest with the released artifact as well. Does it makes sense to download from publicly downloadable link

Example: https://artifacts.opensearch.org/releases/bundle/opensearch/1.2.4/opensearch-1.2.4-linux-x64.tar.gz

We can download from public url , unzip and use the manifest. In that case just the version is required. It also makes sure we are using the manifest for released artifact

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked this idea of using this public link for transparency purpose. Just make sure few things:
This release artifacts matches the build ID that we will be using for each release right?
Also I actually uses the builds manifest instead of the dist manifest from link like https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/$version/$build_number/linux/x64/build/opensearch/manifest.yml; using the dist manifest from the release artifacts will not include some components that won't redistribute anything (e.g. common-util). So we will still need to manually tag for them.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The build manifest contains all components:

  - name: common-utils
    repository: https://github.com/opensearch-project/common-utils.git
    ref: main
    commit_id: 361db4ce3933fe902773df1266fc32cf6b0098bb
    artifacts:
      maven:
        - maven/org/opensearch/common-utils/maven-metadata.xml

I think we should add common-utils into the dist manifest as well, even though it has no redistributable binaries, #1703.

Copy link
Member

Choose a reason for hiding this comment

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

Implemented in #1705

Copy link
Member

Choose a reason for hiding this comment

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

We can use the manifest directly from released artifact now. Would recommend to use that instead of build id. In that case just the version is required to retrieve the manifest.
Thanks!

@@ -1,6 +1,9 @@
lib = library(identifier: 'jenkins@20211123', retriever: legacySCM(scm))

pipeline {
options {
timeout(time: 2, unit: 'HOURS')
Copy link
Contributor

Choose a reason for hiding this comment

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

How long does this job take, in your tests?

Copy link
Member Author

@zelinh zelinh Mar 1, 2022

Choose a reason for hiding this comment

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

It depends on multiple things: the number of components the manifest includes, the size of contents for each repo, github fetching time, etc.. I tested with only three components and it normally took around 2 mins.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then let's change the timeout to be 30mins?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the timeout here is the max time this job possibly takes. I would like to have this longer just in case. Shouldn't be a big deal IMO.

Copy link
Member

Choose a reason for hiding this comment

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

2 hr is a ok time range, I normally have 2x 3x time for max range.

Copy link
Contributor

Choose a reason for hiding this comment

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

3x for 10min is 30mins

Copy link
Member Author

Choose a reason for hiding this comment

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

We won't know if this takes 10 mins or 30 mins or even more. I believe 2 hours should be an acceptable range.

Copy link
Contributor

@abhinavGupta16 abhinavGupta16 Mar 5, 2022

Choose a reason for hiding this comment

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

So, if we say the timeout is 2hrs, we are expecting this job to run for an 1hr atleast?

It's not a blocker for me, but timeouts should be visited carefully as they are the failsafe that unblock nodes in case a job is stuck.

agent {
docker {
label 'Jenkins-Agent-al2-x64-c54xlarge-Docker-Host'
image 'opensearchstaging/ci-runner:centos7-x64-arm64-jdkmulti-node10.24.1-cypress6.9.1-20211130'
Copy link
Member

Choose a reason for hiding this comment

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

Try to see if you can use alpine:latest instead of this full ledge image for testing.
Since this is literally a git command pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

Eh, seems like you need aws cli as well.

Copy link
Member

Choose a reason for hiding this comment

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

If we used the public manifest URL then we wouldn't need aws cli, see comment below.

@zelinh zelinh force-pushed the auto-release-tag branch from afc0497 to 015c9a6 Compare March 1, 2022 20:39
}
}
parameters {
string(
Copy link
Member

Choose a reason for hiding this comment

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

The build manifest contains all components:

  - name: common-utils
    repository: https://github.com/opensearch-project/common-utils.git
    ref: main
    commit_id: 361db4ce3933fe902773df1266fc32cf6b0098bb
    artifacts:
      maven:
        - maven/org/opensearch/common-utils/maven-metadata.xml

I think we should add common-utils into the dist manifest as well, even though it has no redistributable binaries, #1703.

agent {
docker {
label 'Jenkins-Agent-al2-x64-c54xlarge-Docker-Host'
image 'opensearchstaging/ci-runner:centos7-x64-arm64-jdkmulti-node10.24.1-cypress6.9.1-20211130'
Copy link
Member

Choose a reason for hiding this comment

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

If we used the public manifest URL then we wouldn't need aws cli, see comment below.

tests/jenkins/TestReleaseTagJob.groovy Outdated Show resolved Hide resolved
echo "Creating $version release tag for $componetsNumber components in ths manifest"

withCredentials([usernamePassword(credentialsId: "${GITHUB_BOT_TOKEN_NAME}", usernameVariable: 'GITHUB_USER', passwordVariable: 'GITHUB_TOKEN')]) {
for (component in componentsName) {
Copy link
Member

@dblock dblock Mar 3, 2022

Choose a reason for hiding this comment

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

I dislike the implementation in shell a lot, but with the rule of thumb that this is better than what we had (manual process) I vote to merge this as is.

@abhinavGupta16
Copy link
Contributor

abhinavGupta16 commented Mar 3, 2022

I dislike the implementation here, but with the rule of thumb that this is better than what we had (manual process) I vote to merge this as is.

@zelinh - Can you create an issue to look into this in the future? Thank you!

tests/jenkins/TestReleaseTagJob.groovy Outdated Show resolved Hide resolved
tests/jenkins/TestReleaseTagJob.groovy Outdated Show resolved Hide resolved
def push_url = "https://$GITHUB_TOKEN@" + repo.minus('https://')
echo "Tagging $component at $commitID ..."

dir (component) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much better than before! Thanks!

zelinh added 2 commits March 4, 2022 15:33
Signed-off-by: Zelin Hao <[email protected]>
vars/createReleaseTag.groovy Outdated Show resolved Hide resolved
@zelinh zelinh requested a review from abhinavGupta16 March 5, 2022 00:00
Copy link
Contributor

@abhinavGupta16 abhinavGupta16 left a comment

Choose a reason for hiding this comment

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

Worked with @zelinh to add library related shMock commands as a part of lib tester. This removes all the mocks from the library test and job test. It also eliminates the need for the user to write shMock if they intend to use this library.

@zelinh - Approving the PR. However, I would revisit the timeout. I feel under no circumstance it would take 2 hrs, or even 1hr. It just blocks the node unnecessarily and therefore can be reduced

Thanks!

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Unblocking this PR merge. @zelinh is working on #1666 (comment)
Please feel free to merge if that is coming in another PR.
Thanks!

@zelinh zelinh merged commit 46c244b into opensearch-project:main Mar 7, 2022
@zelinh zelinh deleted the auto-release-tag branch March 7, 2022 19:16
@zelinh
Copy link
Member Author

zelinh commented Mar 7, 2022

Issue is created to use public release manifest here #1710. Will be pushing this from another PR.

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.

7 participants