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

Docker Patch Library to support Docker re-release Automation #268

Merged
merged 6 commits into from
Aug 18, 2023

Conversation

Divyaasm
Copy link
Collaborator

@Divyaasm Divyaasm commented Aug 1, 2023

Description

Addressing the Issue

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
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #268 (b339016) into main (f184ed3) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #268      +/-   ##
============================================
+ Coverage     86.63%   86.69%   +0.06%     
  Complexity       27       27              
============================================
  Files            77       78       +1     
  Lines           202      203       +1     
  Branches         11       11              
============================================
+ Hits            175      176       +1     
  Misses           19       19              
  Partials          8        8              
Files Changed Coverage Δ
tests/jenkins/jobs/PatchDockerImage_Jenkinsfile 100.00% <100.00%> (ø)

@Divyaasm Divyaasm force-pushed the dockerpatch branch 2 times, most recently from c8c6f6e to 9aa11ae Compare August 1, 2023 19:40
Signed-off-by: Divya Madala <[email protected]>
Comment on lines 21 to 25
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionnumber"""

sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.build-date"}}' ${docker_image} > time"""

sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > number"""
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to combine these shell commands into above sh""" block.
Also avoid using time as variable name. Can mess up actual clock sometimes.

Copy link
Collaborator Author

@Divyaasm Divyaasm Aug 1, 2023

Choose a reason for hiding this comment

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

Yeah but having all these statements in a single block isn't storing the output value in a variable. That's technically resulting an error. I'll change time variable name. Thanks

Copy link
Member

@gaiksaya gaiksaya Aug 1, 2023

Choose a reason for hiding this comment

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

That doesn't sound right! This might be the culprit https://github.com/opensearch-project/opensearch-build-libraries/pull/268/files#diff-d42288632ebcbc435313fa808e87e49f58461165639396b6ec30cbb928596336R15-R17

I believe by default the shell is zsh and here we are running bash. Also those lines apply only to one command docker pull ${docker_image}. Try removing those and put it under default shell


docker pull ${docker_image}
"""
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionnumber"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionnumber"""
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionNumber"""

Copy link
Member

Choose a reason for hiding this comment

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

Use sh("") if you are trying to do one liner unless you have other reasons.


sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.build-date"}}' ${docker_image} > time"""

sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > number"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > number"""
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > buildNumber"""

Comment on lines 49 to 51
if (artifactUrlX64 == null || artifactUrlARM64 == null) {
echo 'Skipping docker build, one of x64 or arm64 artifacts was not built.'
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Please format the document for proper alignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure Sayali

if (artifactUrlX64 == null || artifactUrlARM64 == null) {
echo 'Skipping docker build, one of x64 or arm64 artifacts was not built.'
} else {
echo 'Trigger docker-build'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo 'Trigger docker-build'
echo 'Triggering docker-build'

]
}

echo 'Trigger docker-copy with tag build date '
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo 'Trigger docker-copy with tag build date '
echo 'Triggering docker-copy with tag build date '

}
}

echo "Trigger docker-scan for ${args.product} version ${inputManifest.build.version}${build_qualifier}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo "Trigger docker-scan for ${args.product} version ${inputManifest.build.version}${build_qualifier}"
echo "Triggering docker-scan for ${args.product} version ${inputManifest.build.version}${build_qualifier}"

]
}

echo 'Trigger docker-promote'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo 'Trigger docker-promote'
echo 'Triggering docker-promote'

}

echo 'Trigger docker-promote'
if(args.rerelease){
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do few more tests before pushing to Prod? For example we do not want to push to prod if high CVEs are detected in above docker scan. @bbarani @peterzhuamazon

Also @Divyaasm please add https://github.com/opensearch-project/opensearch-build/blob/main/jenkins/opensearch/distribution-build.jenkinsfile#L440 wait:true for each job that is being triggered. I think default is true by default but better to mention it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Think we have ignored CVE's presence in this implementation promote the image when Docker-Scan and Docker-Validation(enhancement) are successful . The idea is to re-run the Job every week to align with Base OS Image. @bbarani Correct me if I'm wrong.

Copy link
Member

@bbarani bbarani Aug 1, 2023

Choose a reason for hiding this comment

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

I don't think we can do anything about any CVE at this point in time if it didn't get remediated as part of yum update as the fix needs to come from base OS team. My recommendation is to move forward with available fixes and patch the Docker images on regular basis.

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.

Hi @Divyaasm please read the comment here and considering not copy pasting the entire dockerbuild lib if possible.
I would suggest to re-use the lib or modify with new functions as we discussed before.
https://github.com/opensearch-project/opensearch-build-libraries/blob/main/vars/buildDockerImage.groovy

Thanks.

Comment on lines 14 to 15
sh"""
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Use this:

    sh """#!/bin/bash

Your original implementation will not force bash and will default back to sh.

vars/patchDockerImage.groovy Show resolved Hide resolved

docker pull ${docker_image}
"""
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionnumber"""
Copy link
Member

Choose a reason for hiding this comment

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

Use sh("") if you are trying to do one liner unless you have other reasons.

Comment on lines 23 to 25
sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.build-date"}}' ${docker_image} > time"""

sh """docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > number"""
Copy link
Member

Choose a reason for hiding this comment

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

Why are we keep doing one liners here?


artifactUrlARM64 = "https://ci.opensearch.org/ci/dbc/distribution-build-${args.product}/${version}/${build_number}/linux/arm64/tar/dist/${args.product}/${args.product}-${version}-linux-arm64.tar.gz"

/*slice the time to get date value*/
Copy link
Member

Choose a reason for hiding this comment

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

Use //

vars/patchDockerImage.groovy Show resolved Hide resolved
vars/patchDockerImage.groovy Show resolved Hide resolved
Signed-off-by: Divya Madala <[email protected]>
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.

Hi @Divyaasm this is much better and I add some comments on this.
Please take a look when you have time.

Thanks.

vars/buildDockerImage.groovy Outdated Show resolved Hide resolved
vars/buildDockerImage.groovy Outdated Show resolved Hide resolved
vars/buildDockerImage.groovy Outdated Show resolved Hide resolved
vars/patchDockerImage.groovy Outdated Show resolved Hide resolved
vars/patchDockerImage.groovy Outdated Show resolved Hide resolved
Signed-off-by: Divya Madala <[email protected]>
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.

Hi @Divyaasm please check more comments here.

Thanks.

@@ -6,11 +6,24 @@
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
/**
@param Map[inputManifest] <Required> - path to Input Manifest.
Copy link
Member

Choose a reason for hiding this comment

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

Capital path.

vars/patchDockerImage.groovy Show resolved Hide resolved
vars/patchDockerImage.groovy Show resolved Hide resolved
Comment on lines 22 to 25
docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${docker_image} > versionNumber
docker inspect --format '{{ index .Config.Labels "org.label-schema.build-date"}}' ${docker_image} > time
docker inspect --format '{{ index .Config.Labels "org.label-schema.description"}}' ${docker_image} > buildNumber
docker inspect --format '{{ index .Config.Labels "org.label-schema.version"}}' ${latest_docker_image} > latestVersionNumber
Copy link
Member

Choose a reason for hiding this comment

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

I didnt realize you are creating files here.
You can simply use this to save all the values?

GIT_COMMIT_EMAIL = sh (
    script: 'git --no-pager show -s --format=\'%ae\'',
    returnStdout: true
).trim()

Possibly I miss it in the initial review?

Thanks.

Signed-off-by: Divya Madala <[email protected]>
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.

Great work, thanks.

Divyaasm and others added 2 commits August 18, 2023 11:58
Signed-off-by: Divya Madala <[email protected]>
@gaiksaya gaiksaya merged commit be4e5cd into opensearch-project:main Aug 18, 2023
6 of 7 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 18, 2023
Signed-off-by: Divya Madala <[email protected]>
Signed-off-by: Divya Madala <[email protected]>
(cherry picked from commit be4e5cd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
gaiksaya pushed a commit that referenced this pull request Aug 18, 2023
Signed-off-by: Divya Madala <[email protected]>
Signed-off-by: Divya Madala <[email protected]>
(cherry picked from commit be4e5cd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

4 participants