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

Add sanitize job name helper #183

Merged
merged 2 commits into from
Mar 17, 2020
Merged

Add sanitize job name helper #183

merged 2 commits into from
Mar 17, 2020

Conversation

ericfranz
Copy link
Contributor

@ericfranz ericfranz commented Mar 16, 2020

This addresses the base concerns of #172

You can opt in by setting one or more illegal chars in env var OOD_JOB_NAME_ILLEGAL_CHARS. If you do this then these chars will be replaced with a dash when the helper method is used. The helper method can then be used in the dashboard app when submitting jobs.

This is the simplest and least disruptive change. It omits obvious enhancements like:

  • using squeeze to remove consecutive dashes
  • enabling the replacement char to be configured
  • enabling dash to be specified as illegal
  • enabling adapter or cluster specific modifications
  • providing adapter specific defaults based on documentation of different schedulers

While all of these things are possible, these add more complexity and so this PR postpones that work till we get a new requirements.

@ericfranz ericfranz requested a review from johrstrom March 16, 2020 15:36
@johrstrom
Copy link
Contributor

That seems OK, I think I still prefer ways that would work out of the box for everyone, but I suppose this a greater good situation.

@ericfranz
Copy link
Contributor Author

The only problem with this solution is those methods really don't belong there. Right now there isn't anything that changes from adapter to adapter in regards to this. There is only anticipation of a future need for this. Will sleep on it before merging.

@ericfranz
Copy link
Contributor Author

I added OSC/ondemand#425 to revisit this in the future.

@ericfranz ericfranz merged commit 4b190a2 into master Mar 17, 2020
@ericfranz ericfranz deleted the sanitize_job_name branch March 17, 2020 11:27
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.

2 participants