-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add external dependencies fields to helm remote #263
Add external dependencies fields to helm remote #263
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
97b87b9
to
8e55e8c
Compare
I have read the CLA Document and I hereby sign the CLA |
8e55e8c
to
e38cb7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has my approval! Thanks for the PR
}, | ||
RequiredWith: []string{"external_dependencies_enabled"}, | ||
Description: "An Allow List of Ant-style path expressions that specify where external dependencies may be downloaded from. " + | ||
"By default, this is set to ** which means that dependencies may be downloaded from any external source.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be explicit about the default here with Default: "**",
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no explicit default in remote docker repo: https://github.com/jfrog/terraform-provider-artifactory/blob/master/pkg/artifactory/resource_artifactory_remote_docker_repository.go#L35-L45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be explicit about the default here with
Default: "**",
?
I think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to add a default for a list, tried Default: []string{"**"},
.
1 error occurred:
* resource artifactory_remote_helm_repository: external_dependencies_patterns: Default is not valid for lists or sets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks TF!
Artifactory schema documentation has no default value for externalDependenciesPatterns
so is "**"
the implicit default behavior from the API?
8593e41
to
8e5e682
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick review @alexhung @chb0github! Made a final adjustment, so that the example pattern is equal to the one in https://www.jfrog.com/confluence/display/JFROG/Kubernetes+Helm+Chart+Repositories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the exception on the default value for externalDependenciesPatterns
, this PR is good to go for me.
Since TF doesn't allow default value to be set for TypeList
or TypeSet
, the only question I have is why **
is mentioned in the original description. This suggests either an implicit behavior of Artifactory (or the API), or a mistake in the description.
Having said that, I just read the doc and it clearly states the |
I have been using https://github.com/tenstad/terraform-provider-artifactory/blob/tenstad/pkg/artifactory/resource_artifactory_remote_repository.go#L210-L215 for a while and checked my Artifactory insance. Found a lot of helm remotes with When I now added a new helm remote with terraform (with the provider linked above), it got @alexhung should we update the description, and remove Sorry for not discovering this earlier, I have been used to the |
@tenstad I can confirm the API defaults the |
Closes #259
Tested with Artifactory 7.27.3