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

feat(appset): Add stringTemplate field to spec #11183

Closed
wants to merge 8 commits into from

Conversation

mrmm
Copy link

@mrmm mrmm commented Nov 3, 2022

Closes: #11213

This will introduce a new field stringTemplate that will allow a less restrictive templating of the Application to be generated as the current design is limited by field.

Related discussions:

This work was started in #9873

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@mrmm mrmm marked this pull request as draft November 3, 2022 17:01
@mrmm
Copy link
Author

mrmm commented Nov 3, 2022

Hey, @speedfl I hope you are doing well,

I am struggling with having a good unit test to run for the RenderTemplateParams (that was moved to templating.go file), do you have a suggestion to fix this please 🙏

@speedfl
Copy link
Contributor

speedfl commented Nov 3, 2022

@mrmm are you sure you want to create the method renderWithGoTemplate. Can the Replace in utils do the job ?

Concerning your tests as you are passing the stringTemplate you need to add test with stringTemplate != nil in the utils_test.go

  • One test success
  • One test with UnmarshalStrict failure

You can add a test on yaml templating error

https://github.com/argoproj/argo-cd/blob/master/applicationset/utils/utils_test.go#L60

Something like

template := "hello: {{ .one }}"

	tests = append(tests, struct {
		name        string
		fieldVal    string
		template    *string
		params      map[string]interface{}
		expectedVal string
	}{
		name:        "test",
		fieldVal:    "",
		template:    &template,
		expectedVal: "hello: world",
		params: map[string]interface{}{
			"one":   "world",
			"three": "four",
		},
	})

@mrmm
Copy link
Author

mrmm commented Nov 4, 2022

Hello @speedfl, thanks for the prompt reply,

@mrmm are you sure you want to create the method renderWithGoTemplate. Can the Replace in utils do the job ?

Yes totally, I have just tried to use the work of @rishabh625 (as it was his idea) but using r.Replace does the job (which I have updated in b91759c)

Concerning your tests as you are passing the stringTemplate you need to add test with stringTemplate != nil in the utils_test.go

Thanks for the pointer on how to test, here is my try https://github.com/mrmm/argo-cd/blob/437a9a82fc40e857ec4373297d7a521684bbdc87/applicationset/utils/utils_test.go#L19 but I seem to be running into an Unmarshaling issue for some reason I couldn't find.

(Please let me know if you don't have the time to check this I will gladly stop pinging here 🙏 )


               Error Trace:    templating_test.go:554
               Error:          Received unexpected error:
                               yaml: unmarshal errors:
                                 line 1: field metadata not found in type v1alpha1.Application
                                 line 6: field repoURL not found in type v1alpha1.ApplicationSource
                                 line 7: field targetRevision not found in type v1alpha1.ApplicationSource
                                 line 12: field syncPolicy not found in type v1alpha1.ApplicationSpec
               Test:           TestRenderTemplateParamsStringTemplate/text_template

Edit: found my unmarshal mistake in 437a9a8

@mrmm mrmm force-pushed the feat/gotemplate-and-sprig branch 6 times, most recently from 93d3002 to 437a9a8 Compare November 4, 2022 23:07
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 47.00% // Head: 46.95% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (fc26e2e) compared to base (9fe4ad3).
Patch coverage: 58.82% of modified lines in pull request are covered.

❗ Current head fc26e2e differs from pull request most recent head 21bbdcc. Consider uploading reports for the commit 21bbdcc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11183      +/-   ##
==========================================
- Coverage   47.00%   46.95%   -0.06%     
==========================================
  Files         243      243              
  Lines       40443    40455      +12     
==========================================
- Hits        19012    18995      -17     
- Misses      19520    19546      +26     
- Partials     1911     1914       +3     
Impacted Files Coverage Δ
.../apis/application/v1alpha1/applicationset_types.go 32.43% <ø> (ø)
...cationset/controllers/applicationset_controller.go 61.02% <33.33%> (-2.95%) ⬇️
applicationset/utils/utils.go 77.43% <63.63%> (-3.25%) ⬇️
...licationset/generators/generator_spec_processor.go 64.76% <66.66%> (-1.24%) ⬇️
applicationset/generators/git.go 85.86% <0.00%> (-1.07%) ⬇️
util/settings/settings.go 48.42% <0.00%> (ø)
applicationset/utils/policy.go 0.00% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mrmm
Copy link
Author

mrmm commented Nov 15, 2022

Hey @crenshaw-dev (really sorry for the multiple pings) is this feature still relevant to work on or not?

@adabuleanu
Copy link

@mrmm I encountered a situation that may benefit from this feature. This is described in: argoproj/applicationset#609. I would be very grateful if you can take this scenario into consideration when developing this feature (if you did not do that already).

From your experience is there any existing feature that might help with my use-case? I have been going over lots of argocd docs and tickets and ended up here.

@crenshaw-dev
Copy link
Member

@mrmm this feature is definitely still relevant, I just haven't had a chance to dig into the code yet. Hopefully for 2.7?

@mrmm
Copy link
Author

mrmm commented Dec 6, 2022

@mrmm this feature is definitely still relevant, I just haven't had a chance to dig into the code yet. Hopefully for 2.7?

@crenshaw-dev Thanks for the reply, and sorry again for the multiple pings. I just wanted confirmation from a main contributor before dedicating more time to it 🙏 (Thanks for the confirmation).

I will work on this by the end of the week and the next week, hopefully, will have some tests implemented and refined a little bit of the code.

@crenshaw-dev
Copy link
Member

@mrmm sounds great!

The main thing I'd want to see in this PR (besides working code, tests, the usual :-D) is documentation clarifying when it makes sense to use stringTemplate vs template.

Specifically, template allows the ApplicationSet author to strictly enforce certain field values (like project) without having to think too long about user input validation. stringTemplate opens things up a bit, so it might not make as much sense when an AppSet author wants to clearly lock things down.

@brunocascio
Copy link

Hey there!

I have also this use case where this PR would help: #11589

@mrmm mrmm force-pushed the feat/gotemplate-and-sprig branch 4 times, most recently from ae040d7 to d8553d1 Compare December 16, 2022 16:57
jannfis and others added 2 commits December 16, 2022 18:31
…rgoproj#11239) (argoproj#11724)

* fix: Unbreak operation termination

Signed-off-by: jannfis <[email protected]>

* Revert change to Dockerfile

Signed-off-by: jannfis <[email protected]>

Signed-off-by: jannfis <[email protected]>
Signed-off-by: Ronittos <[email protected]>
* fix: support relative links in OCI tags query response

Pagination for OCI tags retrieval is not supported when the
Link header URI is relative.
According to https://docs.docker.com/registry/spec/api/#pagination
and the therein referenced RFC
https://www.rfc-editor.org/rfc/rfc5988#section-5
relative links should be resolved to the initial request URL

Signed-off-by: detvdl <[email protected]>

* chore: clean up unused prints & assert errors

Signed-off-by: detvdl <[email protected]>

* fix: stop double-escaping repoURL

Signed-off-by: detvdl <[email protected]>

* chore: CodeQL CWE-117 log sanitizing

Signed-off-by: detvdl <[email protected]>

* chore: remove unnecessary error

Signed-off-by: detvdl <[email protected]>

Signed-off-by: detvdl <[email protected]>
Signed-off-by: Ronittos <[email protected]>
@crenshaw-dev
Copy link
Member

I'm personally not going to be able to prioritize this for 2.7.

If you'd like to advocate for the PR, I'd recommend dropping into one of the contributor meetings. https://github.com/argoproj/argoproj#contributing

Sorry I can't help you out this release cycle, I know you've put a lot of work into it!

@crenshaw-dev crenshaw-dev changed the title feat(ApplicationSet): Add stringTemplate field to spec feat(appset): Add stringTemplate field to spec Mar 29, 2023
@StepanKuksenko
Copy link

@mrmm you did a great job I hope you will find time to continue working on this PR to finally deliver the feature

@Gitopolis
Copy link

This would work nicely with image updater annotations. Currently we have to hardcode all the annotation keys, which deprives us the flexibility of image lists and update strategies

@blakepettersson
Copy link
Member

I think this will be superceded with #14893?

@mrmm
Copy link
Author

mrmm commented Oct 7, 2023

For everyone watching this PR, due to personal circumstances I was not able to finish up this feature so sorry 🙏.
Thanks @crenshaw-dev for all the help during this PR and hopefully my next PR to go to fulfillment 👌

@crenshaw-dev
Copy link
Member

@mrmm thank you for all your effort on this! We'll get some version of it merged sooner or later. 🙂

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.

feat(ApplicationSet): Templating - Add possibility of using advanced templating across template field
10 participants