Skip to content
This repository has been archived by the owner on Jul 15, 2024. It is now read-only.

fix: sanitize basename param by replacing unsupported character #436

Merged
merged 7 commits into from
Dec 30, 2021
Merged

fix: sanitize basename param by replacing unsupported character #436

merged 7 commits into from
Dec 30, 2021

Conversation

missingcharacter
Copy link
Contributor

Sanitize the cluster name in accordance with the below rules:

  1. contain no more than 253 characters
  2. contain only lowercase alphanumeric characters, '-' or '.'
  3. start and end with an alphanumeric character

Closes #306

@CLAassistant
Copy link

CLAassistant commented Dec 14, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

How would you feel about adding a new variable called path.basenameNormalized to mirror the changes here? #390

If we go the direction of adding a new variable, I think it should also be documented (along with the cluster generator variable if you have some extra time 😄 ).

@jgwest just pinging with this thought: if we were to introduce some more sophisticated templating tool (e.g. expr), this would be a use case.

Copy link
Contributor

@thecubed thecubed left a comment

Choose a reason for hiding this comment

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

Hi @missingcharacter !
Per our discussion earlier, here's some suggestions for the documentation.

This does seem to me like it might be useful for this project to switch to using a template generator that allows filters so one could write something like {{ basename | replace(".","_") }} akin to jinja2 or Go templates.

ApplicationSets appears to use https://github.com/valyala/fasttemplate (https://github.com/argoproj-labs/applicationset/blob/master/pkg/utils/util.go#L16) which doesn't support functions or filters, though.

That might be a task for someone else to take on at a later point though.

docs/Generators-Cluster.md Outdated Show resolved Hide resolved
docs/Generators-Git.md Outdated Show resolved Hide resolved
docs/Generators-Git.md Outdated Show resolved Hide resolved
@missingcharacter
Copy link
Contributor Author

Thanks @crenshaw-dev

@crenshaw-dev
Copy link
Member

@jgwest can you take a look?

@missingcharacter
Copy link
Contributor Author

@jgwest am I missing something for workflows to be approved?

@wtam2018
Copy link
Collaborator

please resolve conflicts @missingcharacter

@missingcharacter
Copy link
Contributor Author

missingcharacter commented Dec 30, 2021

@wtam2018 I did, could you please approve the workflows?

Also, I ran my changes on my repo and they seem good https://github.com/missingcharacter/applicationset/runs/4668321217?check_suite_focus=true

@wtam2018 wtam2018 self-requested a review December 30, 2021 22:37
Copy link
Collaborator

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @missingcharacter

@wtam2018 wtam2018 merged commit 3013531 into argoproj:master Dec 30, 2021
@missingcharacter
Copy link
Contributor Author

Thanks @wtam2018 🙇🏽‍♂️

@missingcharacter missingcharacter deleted the sanitize-basename branch December 31, 2021 00:00
ishitasequeira pushed a commit to ishitasequeira/applicationset that referenced this pull request Jan 11, 2022
…proj#436)


Co-authored-by: Tyler Montgomery <tylerfixer@thecubed.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanitize application names
6 participants