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.

Sign up for GitHub

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

Jump to bottom

Support for Distribution path mapping with AQL #1031

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Support for Distribution path mapping with AQL #1031

merged 2 commits into from
Nov 27, 2023

Conversation

YardenEdery
Copy link
Contributor

@YardenEdery YardenEdery commented Nov 14, 2023 •
edited
Loading

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 14, 2023 •
edited
Loading

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

@YardenEdery YardenEdery changed the base branch from master to dev November 15, 2023 16:18
@YardenEdery
Copy link
Contributor Author

Not sure about adding tests here, there are none related to distribution

@github-actions GitHub Actions
Copy link
Contributor

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

@eyalbe4 eyalbe4 self-requested a review November 22, 2023 16:11
eyalbe4
eyalbe4 requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@eyalbe4 eyalbe4 left a comment

Choose a reason for hiding this comment

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

Let's add a complete description for this PR.

common/spec/specfiles.go Outdated
@@ -194,6 +197,9 @@ func ValidateSpec(files []File, isTargetMandatory, isSearchBasedSpec bool) error
isBypassArchiveInspection, _ := file.IsBypassArchiveInspection(false)
isTransitive, _ := file.IsTransitive(false)

if isPathMapping && !isAql {
return errorutils.CheckErrorf("mappings supported only with Aql")
Copy link
Contributor

Choose a reason for hiding this comment

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

mappings supported only with Aql
-->
path mapping is supported only with Aql

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure because "mappings" is the interface to the user, they will use "mappings" in their spec.json and I saw in the code you treat it as PathMappings so I kept the same conventions, and thought that the error should show the user what he needs to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. NP.

common/spec/specfiles.go Outdated
Comment on lines 215 to 220
if isPathMapping && isTarget {
return fileSpecValidationError("mappings", "target")
}
if isPathMapping && isPattern {
return fileSpecValidationError("mappings", "pattern")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's place the above line right after the following code for better readability -

if isPathMapping && !isAql {
	return errorutils.CheckErrorf("mappings supported only with Aql")
}

@eyalbe4 eyalbe4 changed the title Add distribution path mappings Support for Distribution path mapping with AQL Nov 22, 2023
@eyalbe4 eyalbe4 added the new feature Automatically generated release notes label Nov 22, 2023
@YardenEdery YardenEdery requested a review from eyalbe4 November 26, 2023 12:12
yahavi
yahavi requested changes Nov 26, 2023
View reviewed changes
common/spec/specfiles.go Outdated
@@ -41,6 +41,7 @@ func CreateSpecFromFile(specFilePath string, specVars map[string]string) (spec *

type File struct {
Aql utils.Aql
PathMapping utils.PathMapping `json:"mappings,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave it "pathMapping"? Or use "mapping"?

Suggested change
PathMapping utils.PathMapping `json:"mappings,omitempty"`
PathMapping utils.PathMapping

Copy link
Contributor Author

@YardenEdery YardenEdery Nov 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

done

yahavi
yahavi previously approved these changes Nov 27, 2023
View reviewed changes
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.

LGTM

@yahavi yahavi self-requested a review November 27, 2023 11:43
@yahavi yahavi dismissed their stale review November 27, 2023 11:44

Let's fix the static code analysis before merging the code

@yahavi yahavi merged commit daea501 into jfrog:dev Nov 27, 2023
7 of 8 checks passed
@yahavi yahavi mentioned this pull request Nov 27, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyalbe4 eyalbe4 Awaiting requested review from eyalbe4

@yahavi yahavi Awaiting requested review from yahavi

Assignees
No one assigned
Labels
new feature Automatically generated release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@YardenEdery @Yarden-Edery @eyalbe4 @yahavi