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

fix(templates/../spin.toml): skewer spin application name like a kebab #2531

Merged
merged 1 commit into from
May 27, 2024

Conversation

brehen
Copy link
Contributor

@brehen brehen commented May 24, 2024

TLDR: Kebab skewers are great 🍡


With my Master's thesis handed in where I built a Temu/Wish knockoff of your products, I finally have time to try out your products proper. Howver, I encountered a bug while naively naming my spin project.

I'm not sure if omitting the kebab skewer from the generated spin.toml file was intentional or not, but I was met with this nifty error while trying to deploy the app I named with whitespaces:

Error: App name 'is it wednesday yet my dudes' contains characters that are not allowed. It may contain only letters, numbers, dashes and underscores

The error disappeared when I updated the name in my spin.toml to include the skewer, and I was able to deploy as expected.

I noticed that the Cargo.toml and folder name replaced the whitespace correctly, but not spin.toml.application.name. No one would be stupid enough to name their project with whitespace, eh? (Hi, it's a me! 👋 👨‍💼)

After digging through the source code that I'm now more suited for understanding after building Nebula (self-plug), I noticed that the kebab_case piping were missing in all the spin.toml templates. You're skewering the other other instances of project-name in here though, so I figured it might have been a mishap?

I'm able to build and deploy to Fermyon Cloud after installing the templates from the local git repository where I've update the templates.

My LSP complained about some of the .toml templates missing a newline between [component.name] and [component.name.build]. Let me know if I should revert these newlines.

@itowlson
Copy link
Collaborator

Hmm, local Spin allows whitespace in application names, but it looks like Fermyon Cloud (or the Fermyon Cloud client) doesn't. We should make them consistent but will need to make a decision on which is "right." Thanks for flagging this!

@bacongobbler @lann What do you reckon is the desired behaviour? Is the ban on whitespace in names in cloud to do with registry IDs and URLs and stuff?

@brehen Yes, please revert the newlines around the build sections. Each component should be its own visual "chunk." For motivation, imagine eyeballing through a larger manifest trying to find a component: having subtables lke [component.*.variables] and [component.*.build] as their own visual sections would double the number of sections you had to look though. (Of course you are free to reformat your files how you like! But this is why the templates are why they are.)

Signed-off-by: Marius Kluften <marius@kluften.dev>
@brehen
Copy link
Contributor Author

brehen commented May 27, 2024

Glad to be of help!

I agree, for templates it makes sense to keep them compact for the templates. I've reverted my commit, removed the newlines and pushed the new squeaky clean commit now. 🧼

@bacongobbler
Copy link
Contributor

bacongobbler commented May 27, 2024

Is the ban on whitespace in names in cloud to do with registry IDs and URLs and stuff?

For the most part, yes. The app name is used to generate the hostname for the domain. It's also referenced in certain parts of our infrastructure.

It would be fine if Spin itself were to sanitize the name ahead of any interactions with the Fermyon Cloud. However, if there are any plugins that read spin.toml to fetch the app name for lookups in the Cloud API, it'd have to do the same.

We could do some work on the Cloud API side to normalize the name as part of app creation and use those names interchangeably. I'm not sure we're planning to do that work any time soon however.

@itowlson
Copy link
Collaborator

Thanks @bacongobbler. Sounds like we should kebab the name in the template then, even if local Spin doesn't enforce it: that way it's not a breaking change but minimises the chances of new users hitting deployment problems.

cc @karthik2804 for the Python and JS templates

Copy link
Collaborator

@itowlson itowlson 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 for your patience while we figured things out!

@itowlson itowlson merged commit 5bb2ca9 into spinframework:main May 27, 2024
17 checks passed
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