Skip to content

Conversation

@agrasth
Copy link
Collaborator

@agrasth agrasth commented May 11, 2025

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the dev branch.
  • I used gofmt for formatting the code before submitting the pull request.

Add --validate-sha flag for Docker push command

Description

This PR adds support for the new --validate-sha flag to the Docker push command. When enabled, the flag allows the CLI to use the image's SHA digest for validation instead of the tag name during Docker push operations. This is particularly useful when pushing to virtual repositories where the tag might exist with different content in higher priority repositories.

Changes

  • Added flag definition in cliutils/flagkit/flags.go
  • Added the flag to the ContainerPush command flags in the commandFlags map
  • Modified commands/container/containermanagerbase.go to add validateSha field and methods
  • Updated commands/container/push.go to implement the SHA-based validation logic
  • Added unit tests for the new functionality

Testing

  • Added comprehensive unit tests for the flag functionality
  • Manually tested the flag with both old and new CLI syntax
  • Verified behavior when pushing to repositories with existing tags
  • Confirmed build info collection works correctly with the flag

Related PRs

@agrasth agrasth force-pushed the feature/docker-validate-sha branch 9 times, most recently from 7e9a34e to 2eb0fd6 Compare May 16, 2025 22:52
@agrasth agrasth force-pushed the feature/docker-validate-sha branch from 2eb0fd6 to c7c68f1 Compare May 19, 2025 06:03
Comment on lines 139 to 142
if err != nil || buildInfoModule == nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

handle both err and buildInfoModule conditions separately

if err != nil {
	return errorutils.CheckError(fmt.Errorf("failed to build module info: %w", err))
}
if buildInfoModule == nil {
    return errorutils.CheckErrorf("build info module is nil")
}

if pc.IsValidateSha() {
log.Info("Performing SHA-based validation for Docker push...")
// Get image SHA from the container manager
imageSha2, err := cm.Id(pc.image)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
imageSha2, err := cm.Id(pc.image)
imageSha256, err := cm.Id(pc.image)

is it imageSHA256 ?

return err
}
if err = build.SaveBuildInfo(buildName, buildNumber, pc.BuildConfiguration().GetProject(), buildInfoModule); err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return err
return errorutils.CheckError(fmt.Errorf("failed to save build info: %w", err))

Comment on lines 150 to 156
_, err = remoteBuilder.Build("")
if err != nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_, err = remoteBuilder.Build("")
if err != nil {
return err
}
if _, err = remoteBuilder.Build(""); err != nil {
return errorutils.CheckError(fmt.Errorf("failed to build summary info: %w", err))
}

nit picking


// Docker specific commands flags
skipLogin: components.NewBoolFlag(skipLogin, "[Default: false] Set to true if you'd like the command to skip performing docker login.", components.WithBoolDefaultValueFalse()),
validateSha: components.NewBoolFlag(validateSha, "[Default: false] Set to true to enable SHA validation during Docker push.", components.WithBoolDefaultValueFalse()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

flag documentation has to be updated in jfrog/jfrog-documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will add this there as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@agrasth agrasth force-pushed the feature/docker-validate-sha branch from de829b9 to 342dbcf Compare May 23, 2025 22:29
@agrasth agrasth force-pushed the feature/docker-validate-sha branch from 08d064f to 1e1af0b Compare May 27, 2025 09:42
@agrasth agrasth added the safe to test Approve running integration tests on a pull request label May 27, 2025
@agrasth agrasth requested a review from bhanurp May 27, 2025 09:48
@agrasth agrasth merged commit 0361d8f into jfrog:main May 27, 2025
9 checks passed
@agrasth agrasth added the improvement Automatically generated release notes label May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Automatically generated release notes safe to test Approve running integration tests on a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants