Skip to content

Updates schema to enforce names as required RFC1123 values #768

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

Conversation

cdavernas
Copy link
Member

Many thanks for submitting your Pull Request ❤️!

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Roadmap
  • Use Cases
  • Community
  • TCK
  • Other

Discussion or Issue link:
#671 #685

What this PR does / why we need it:

  • Enforce names of states, events, functions, actions, branches and conditions to be required and to follow the RFC1123 DNS label format.

This was linked to issues May 26, 2023
@cdavernas cdavernas changed the title Updates schemas to enforce names as required RFC1123 values Updates schema to enforce names as required RFC1123 values May 26, 2023
@ricardozanini ricardozanini removed the request for review from antmendoza May 27, 2023 16:20
specification.md Outdated

Identifiable components of a workflow definition, such as states, actions, branches, events and functions define a required non-null `name` property which follows the DNS label names as defined by [RFC 1123](https://datatracker.ietf.org/doc/html/rfc1123).

In other words, `names` must be lowercased, start and end with an alphanumeric character, and contain only alphanumeric characters or '-' (dash) character.
Copy link
Contributor

Choose a reason for hiding this comment

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

RFC 1123 does not include any lowercasing requirement; did you intend to make this a further restriction?

I think some text is also warranted regarding RFC 5890 R-LDH labels (whose third and fourth characters are both "-") such as xn--bcher-kva (which is further an XN-label and valid A-label representing "bücher") and we--like--it (which is neither but is still subject to the "MUST NOT be processed as ordinary LDH labels by IDNA-conforming programs" restriction).

I don't think it matters whether they are allowed or rejected, but some software does treat them specially so it's worth being explicit.

Copy link
Member Author

@cdavernas cdavernas May 27, 2023

Choose a reason for hiding this comment

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

@gibson042 I thought it did, or at least does it's k8s flavor:

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#:~:text=RFC%201123%20Label%20Names&text=in%20RFC%201123.-,This%20means%20the%20name%20must%3A,start%20with%20an%20alphanumeric%20character

Anyways, I do think adding the lowercase restriction is a must, as i believe it could help ensure invariance, especially on exotic systems.

It's not however a strong preference, and I'm obviously open to any and all opinion for and against ;)

As for the double dashes, I tend to believe it's safe to forbid its use altogether, like with the k8s regex I'm using in the PR. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

@cdavernas Thanks for the updates!

@cdavernas
Copy link
Member Author

@gibson042 No, thanks to you for both the comments and the suggested edits ❤️

@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change impacts SDK 🔧 area: spec Changes in the Specification labels May 30, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@ricardozanini ricardozanini merged commit 4dd66f0 into serverlessworkflow:main Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: feature New feature or request. Impacts in a minor version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce action name Enforce naming conventions
3 participants