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

RFC: valid identifiers #40

Merged
merged 6 commits into from Jun 24, 2020
Merged

RFC: valid identifiers #40

merged 6 commits into from Jun 24, 2020

Conversation

vito
Copy link
Member

@vito vito commented Nov 13, 2019

vito added 2 commits Nov 13, 2019
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
The decision to allow '.' feels somewhat arbitrary but I feel is easily
justifiable from the standpoint that it is often used for CI/CD related
things like version numbers or a DNS name identifying a deploy target.
It is also one of the few allowable special characters that does not
require any additional URL encoding.

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

# Open Questions

* Do we want to support a 'display name'?

Choose a reason for hiding this comment

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

For my personal circumstance, I don't need this; however, supporting a display name is probably a requirement to provide an upgrade path for a lot of users. People get attached to their personal preferences and you risk admins facing internal political wrath in their organizations over minor quibbles like this.

040-valid-identifiers/proposal.md Outdated Show resolved Hide resolved
vito added 3 commits Nov 14, 2019
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@topherbullock
Copy link
Member

@topherbullock topherbullock commented Dec 12, 2019

This would break Christmas
https://twitter.com/topherbullock/status/950411039954399232

You're a mean one, Mister @vito

@nebhale
Copy link

@nebhale nebhale commented Apr 28, 2020

I’m pretty wary of this change. First, I am sympathetic to the technical challenges that arbitrary naming has to parsing and other programmatic processing of the identifiers; it’s a bear and having an unambiguous syntax is not only useful, but preferable.

However, the strict limitation of only Unicode characters plus a pair of word separators (side note, it’s nearly impossible to find a concise listing of what counts as a Unicode “letter”) is a massive regression in visual differentiability. Take the following examples from my own pipelines which contain a great many, very similar resources often times based on different artifacts from the same root project.

dependency-github.com-paketo-buildpacks-azure-application-insights
module-github.com-paketo-buildpacks-azure-application-insights
package-github.com-paketo-buildpacks-azure-application-insights

Compare those names to an equivalent with an idealized set of names

dependency:github.com/paketo-buildpacks/azure-application-insights
module:github.com/paketo-buildpacks/azure-application-insights
package:github.com/paketo-buildpacks/azure-application-insights

Or even a slightly sanitized version I’ve used (because it’s not just that / is a problem, it’s totally broken in parts of fly)

dependency:github.com|paketo-buildpacks|azure-application-insights
module:github.com|paketo-buildpacks|azure-application-insights
package:github.com|paketo-buildpacks|azure-application-insights

Both of the latter two options are more understandable both in what “type” (dependency, module, package) they are as well as what they refer to (https://github.com/paketo-buildpacks/azure-application-insights)

A display name (now removed from the RFC) is one option here, but I think it’s likely nearly redundant for many identifiers where the majority of characters would be identical, but certain separator characters would be subbed back in for dashes; not a very satisfying way of writing pipeline definitions.

Instead I’d propose that the character set should be loosened to include the common separator characters that are not ambiguous and problematic in the way that / is. There is some concern about the need to quote in a terminal, but that’s a common practice that is familiar to all terminal users and doesn’t feel like a concern that a primarily server-side and web-interfaced application should be ranking strongly.

@vito
Copy link
Member Author

@vito vito commented May 5, 2020

@nebhale Good point! I don't think super-long-pipeline-names-with-embedded-values is a pattern we really want though - it feels more like a workaround in absence of proper grouping and labeling, which is what instanced pipelines (#34) intends to achieve:

$ fly get-pipeline -p dependency -i repo:github.com/paketo-buildpacks/azure-application-insights

I'll update the RFC to mention this. 🙂

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants