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: Allow Identifier's in custom host requirements. #565

Merged

Conversation

erico-aws
Copy link
Contributor

Fixes: #553

What was the problem/requirement? (What/Why)

Users could not enter identifiers in the name field of custom host requirements. It was restricted to just letters, numbers, dashes and underscores up to 99 characters. This doesn't match the attributes of an Identifier as outlined in the OpenJD specification.

What was the solution? (How)

  1. I removed the reserved "worker" identifier from the attribute and amount prefixes.
  2. I added an Identifier regex. The 2 name line edits now use this to check that the value entered is an Identifier or a series of Identifiers separated by periods.
  3. Check the first Identifier entered to make sure that it's not one of the reserved first Identifiers.
  4. Check that the name doesn't end with a period.

What is the impact of this change?

Names for custom host requirements should work now and should be validated correctly.

How was this change tested?

Added Unit tests for the changes.

Deployed changes to Blender for testing to make sure it worked as expected.

Was this change documented?

No.

Does this PR introduce new dependencies?

This library is designed to be integrated into third-party applications that have bespoke and customized deployment environments. Adding dependencies will increase the chance of library version conflicts and incompatabilities. Please evaluate the addition of new dependencies. See the Dependencies section of DEVELOPMENT.md for more details.

  • This PR adds one or more new dependency Python packages. I acknowledge I have reviewed the considerations for adding dependencies in DEVELOPMENT.md.
  • This PR does not add any new dependencies.

Is this a breaking change?

No.

Does this change impact security?

No.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@erico-aws erico-aws requested a review from a team as a code owner January 21, 2025 15:16
@erico-aws erico-aws force-pushed the identifiers_in_host_requirements branch from a414115 to c63fa55 Compare January 27, 2025 20:36
@erico-aws erico-aws requested a review from a team as a code owner January 27, 2025 20:36
Copy link
Contributor

@epmog epmog left a comment

Choose a reason for hiding this comment

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

one little nit, took it for a spin, looks good! Ship it!

@@ -72,11 +73,19 @@
)

CUSTOM_CAPABILITY_NAME_REGEX = "^(\\.[a-zA-Z][a-zA-Z0-9]{0,63})+$"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this wasn't and still isn't actually used anywhere so we can actually remove this.

@erico-aws erico-aws merged commit 519888b into aws-deadline:mainline Jan 29, 2025
25 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.

Bug: custom requirements are broken in the GUI submitter host requirements
3 participants