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 for Distribution path mapping with AQL #861

Merged
merged 4 commits into from
Nov 26, 2023
Merged

Support for Distribution path mapping with AQL #861

merged 4 commits into from
Nov 26, 2023

Conversation

YardenEdery
Copy link
Contributor

@YardenEdery YardenEdery commented Nov 14, 2023

  • 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.

Added support of "mappings" property to file spec in order to have the ability to control where to distribute the artifacts that are being added dynamically when using AQL in the release bundle that is being created.

When looking for docker artifacts using AQL, the release bundle also includes, in the distribution server backend, all the layers for a manifest.json and all of the manifest.json files and their layers if its a list.manifest.json

From now on the "mappings" will take affect on those none specified added manifests and layers and will allow the users a native way of creating the file spec for distribution commands.

FYI This couldn't be achieved using "target" for the list.manifest.json example.

Copy link
Contributor

github-actions bot commented Nov 14, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@YardenEdery
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

1 similar comment
@Yarden-Edery
Copy link

I have read the CLA Document and I hereby sign the CLA

@Yarden-Edery
Copy link

recheck

@yahavi yahavi added the safe to test Approve running integration tests on a pull request label Nov 19, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 19, 2023
Copy link
Contributor

👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.


@eyalbe4 eyalbe4 added new feature Automatically generated release notes and removed improvement Automatically generated release notes labels Nov 22, 2023
@eyalbe4 eyalbe4 changed the title Add distribution path mappings Distribution path mapping support Nov 22, 2023
@eyalbe4 eyalbe4 self-requested a review November 22, 2023 15:36
@eyalbe4 eyalbe4 added the safe to test Approve running integration tests on a pull request label Nov 22, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 22, 2023
@eyalbe4
Copy link
Contributor

eyalbe4 commented Nov 22, 2023

Thanks for creating this PR @YardenEdery!
Can you please add full description for this PR?

Comment on lines 399 to 407
// Make sure <RtTargetRepo>/b.out does exist in Artifactory
searchParams := artifactoryServices.NewSearchParams()
searchParams.Pattern = getRtTargetRepo() + "b.out"
reader, err := testsSearchService.Search(searchParams)
assert.NoError(t, err)
readerCloseAndAssert(t, reader)
length, err := reader.Length()
assert.NoError(t, err)
assert.Equal(t, 1, length)
Copy link
Contributor

Choose a reason for hiding this comment

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

The above code lines seem to be duplicated multiple times in this file. Let's move into a function and reuse the code.

Comment on lines 389 to 397
// Distribute release bundle
distributeBundleParams := distribution.NewDistributeReleaseBundleParams(bundleName, bundleVersion)
distributeBundleParams.DistributionRules = []*distribution.DistributionCommonParams{{SiteName: "*"}}
testsBundleDistributeService.Sync = true
// On distribution with path mapping, the target repository cannot be auto-created
testsBundleDistributeService.AutoCreateRepo = false
testsBundleDistributeService.DistributeParams = distributeBundleParams
err = testsBundleDistributeService.Distribute()
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The above code lines seem to be duplicated multiple times in this file. Let's move into a function and reuse the code.

@eyalbe4 eyalbe4 added the safe to test Approve running integration tests on a pull request label Nov 22, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 22, 2023
@eyalbe4 eyalbe4 changed the title Distribution path mapping support Support for Distribution path mapping with AQL Nov 22, 2023
createBundleParams.SpecFiles = []*utils.CommonParams{
{
Aql: utils.Aql{
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\": \"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"b.in\"}}]}]}",
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
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\": \"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"b.in\"}}]}]}",
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\":\"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"b.in\"}}]}]}",

createBundleParams.SpecFiles = []*utils.CommonParams{
{
Aql: utils.Aql{
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\": \"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"*.in\"}}]}]}",
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
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\": \"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"*.in\"}}]}]}",
ItemsFind: "{\"$or\":[{\"$and\":[{\"repo\":{\"$match\":\"" + strings.TrimSuffix(getRtTargetRepo(), "/") + "\"},\"name\":{\"$match\":\"*.in\"}}]}]}",


createBundleParams.SignImmediately = true
summary, err := testsBundleCreateService.CreateReleaseBundle(createBundleParams)
if !assert.NoError(t, err) {
Copy link
Member

Choose a reason for hiding this comment

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

For better readability and shorter code, let's not use "ifs" in tests. The "assert" and/or "require" alone should help us to eliminate them.

@YardenEdery YardenEdery requested a review from yahavi November 26, 2023 12:12
@yahavi yahavi added the safe to test Approve running integration tests on a pull request label Nov 26, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 26, 2023
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Well done @Yarden-Edery!
Thanks.

@yahavi yahavi merged commit 14887b8 into jfrog:dev Nov 26, 2023
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants