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

Minor bugfix for signArtifacts.groovy #1437

Merged

Conversation

abhinavGupta16
Copy link
Contributor

Signed-off-by: Abhinav Gupta abhng@amazon.com

Description

Currently, the pgp key for signing is only imported if the file is downloaded. We only need to download the file ones, however, we need to import the key if the file already exists or when we download the file.

Issues Resolved

pgp key failure will not happen for preexisting pgp key

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.

Signed-off-by: Abhinav Gupta <abhng@amazon.com>
@abhinavGupta16 abhinavGupta16 added the bug Something isn't working label Jan 7, 2022
@abhinavGupta16 abhinavGupta16 self-assigned this Jan 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #1437 (9d6a8d8) into main (84d8da0) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1437      +/-   ##
============================================
- Coverage     94.45%   94.43%   -0.03%     
  Complexity       11       11              
============================================
  Files           140      141       +1     
  Lines          3085     3071      -14     
  Branches         10        8       -2     
============================================
- Hits           2914     2900      -14     
  Misses          164      164              
  Partials          7        7              
Impacted Files Coverage Δ
tests/jenkins/jobs/SignArtifacts_Jenkinsfile 100.00% <ø> (ø)
...bs/PrintArtifactDownloadUrlsForStaging_Jenkinsfile 100.00% <100.00%> (ø)

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 84d8da0...9d6a8d8. Read the comment docs.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Since this was a bug, lets add unit tests to confirm it works as expected

vars/signArtifacts.groovy Outdated Show resolved Hide resolved
@@ -30,8 +30,9 @@ void call(Map args = [:]) {
}

void importPGPKey(){
keyPath = "$WORKSPACE/opensearch.pgp"
Copy link
Member

Choose a reason for hiding this comment

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

Is this keyPath necessary? I think we are justing taking the gpg import out of the if bracket.

Copy link
Contributor Author

@abhinavGupta16 abhinavGupta16 Jan 8, 2022

Choose a reason for hiding this comment

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

I believe so, yes. I realized that the same path is being used at multiple places. So it is better to have in a variable. This makes it less error prone (for any changes in the path in future) and easy to follow.

Signed-off-by: Abhinav Gupta <abhng@amazon.com>
@abhinavGupta16
Copy link
Contributor Author

abhinavGupta16 commented Jan 8, 2022

Since this was a bug, lets add unit tests to confirm it works as expected

I have added a test case for it. I have also changed the test case to be a unit test case only signArtifacts lib and added another test case for the actual job.

I believe we should do this for every jenkins job we have in the project. It would make sure the actual job test case fails if anyone makes changes to it. This should definitely make our jobs more robust against accidental changes.

Signed-off-by: Abhinav Gupta <abhng@amazon.com>
Signed-off-by: Abhinav Gupta <abhng@amazon.com>
Signed-off-by: Abhinav Gupta <abhng@amazon.com>
Signed-off-by: Abhinav Gupta <abhng@amazon.com>
Signed-off-by: Abhinav Gupta <abhng@amazon.com>
@@ -0,0 +1,33 @@
sign-standalone-artifacts.run()
Copy link
Member

Choose a reason for hiding this comment

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

Please move this file into the .../tests/data path with the other files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data was being used to store dummy files that are required for tests. I have created a separate folder to hold regression txt files for jenkins job, tests/jenkins/jenkinsjob-regression-files. Let me know if the name of the dir works

Signed-off-by: Abhinav Gupta <abhng@amazon.com>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for updating the path on those tests, looks good

@dblock dblock merged commit 1f2608b into opensearch-project:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants