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

Correct default in external dependencies patterns description #267

Merged

Conversation

tenstad
Copy link
Contributor

@tenstad tenstad commented Jan 13, 2022

from any external source.
dependencies may be downloaded from. By default, this is an empty list which means that no dependencies may be downloaded
from external sources. Note that the official documentation states the default is '**', which is correct when creating
repositories in the UI, but incorrect for the API.
Copy link
Contributor

Choose a reason for hiding this comment

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

WOW!!!!!! I mean, I am shocked but not suprised.

@chb0github
Copy link
Contributor

@alexhung - Here's the open question: We can set the default in TF to match the behavior of the UI **. What do you think?

@alexhung
Copy link
Member

@chb0github We can do that too. Probably provide a better user experience, at the cost of us potentially need to keep in sync with any changes of behavior in the UI.

@tenstad
Copy link
Contributor Author

tenstad commented Jan 13, 2022

How would you define the default value when TF schemas does not (AFAIK) support default values for list elemets? Some additional logic to make it ["**"] if it is undefined?

The defaults (API/UI) seem to be equal for remote docker repositories as well. Probably the case for all types and not just helm.

@chb0github
Copy link
Contributor

TF allows you to set a default. In addition, we can explicitly do it in code. Either way, It's easy

@chb0github
Copy link
Contributor

chb0github commented Jan 13, 2022

@alexhung - a users expectation is one of consistency - the delta between the UI and the API (the API documentation is not nearly as well kept) would definitely be problematic. I mean, where do you think I got all the description information for the fields? I literally ripped it from the UI tooltip

I am ok with approving this PR as it at least makes the documentation conform to the behavior of the APIs. But then, another ticket should be create to address the discrepency

@tenstad
Copy link
Contributor Author

tenstad commented Jan 13, 2022

I had some trouble defining a list default, as described here: #263 (comment). But sure, if you know how to do it, it is definitely an option.

Agree that it is confusing when the terraform provider defaults differ from the official documentation.

@chb0github
Copy link
Contributor

The difference isn't different from the official docs - I wouldn't consider the UI tooltip "official" - but the behavior observed when things are done through the UI is crucial and effectively becomes the standard.

@tenstad
Copy link
Contributor Author

tenstad commented Jan 13, 2022

I read https://www.jfrog.com/confluence/display/JFROG/Kubernetes+Helm+Chart+Repositories when trying to get external dependencies working for my helm remote. For Pattern Allow List it states By default, this is set to ** which means that dependencies may be downloaded from any external source. I guess it assumes creation through UI. Tried to verify with https://www.jfrog.com/confluence/display/JFROG/Repository+Configuration+JSON, but there (for remote) externalDependenciesPatterns only applies to Docker repositories.

@chb0github
Copy link
Contributor

@alexhung - terraform doesn't complain about

	"external_dependencies_patterns": {
		Type:     schema.TypeList,
		Optional: true,
		ForceNew: true,
		DefaultFunc: func() (interface{}, error) {
			return []string{"**"}, nil
		},
		Elem: &schema.Schema{
			Type: schema.TypeString,
		},
		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.",
	},

But I don't see the value being propagated either

@tenstad
Copy link
Contributor Author

tenstad commented Jan 13, 2022

Sadly, yeah :/ Here is an issue for it: hashicorp/terraform-plugin-sdk#142

@alexhung
Copy link
Member

@chb0github I'm not disagreeing with you at all :) I think we should be consistent, just need to acknowledge that there is a cost to keep this consistent with the UI.

@chb0github
Copy link
Contributor

@tenstad - so you have to declare an empty block? I mean, at that point, just declare a value!

It can be done in code too:

if ok,val := d.get("external_dependencies_patterns"); !ok {
    payload.ExternalDependencyPatterns = []string{"**"}
}

Not so sexy, but it would work

@chb0github
Copy link
Contributor

@alexhung - your call to merge this

@alexhung
Copy link
Member

@tenstad I'm going to merge this since this accurately reflects the behavior of the provider currently. I'll create a JIRA ticket for the wish to match UI behavior.

@alexhung alexhung merged commit c18f319 into jfrog:master Jan 18, 2022
maheshjfrog added a commit that referenced this pull request Mar 16, 2022
maheshjfrog added a commit that referenced this pull request Mar 16, 2022
maheshjfrog added a commit that referenced this pull request Mar 16, 2022
maheshjfrog added a commit that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants