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

Updated build_requirejs string check #34028

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Updated build_requirejs string check #34028

merged 1 commit into from
Jan 24, 2024

Conversation

orangejenny
Copy link
Contributor

Technical Summary

Cherry-picked from another branch that I'm simplifying. @millerdev I think this was PR feedback from you a while back.

Safety Assurance

Safety story

Tiny change to a management command that's part of the deploy process.

Automated test coverage

no

QA Plan

no

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 24, 2024
Copy link
Contributor

@esoergel esoergel left a comment

Choose a reason for hiding this comment

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

Seems safe enough, but what's the impetus for the change?

@orangejenny
Copy link
Contributor Author

old feedback, rationale was probably that it doesn't make sense to do a regex search and pass it a plain string rather than a regex

@orangejenny orangejenny merged commit 0473c03 into master Jan 24, 2024
13 checks passed
@orangejenny orangejenny deleted the jls/string-check branch January 24, 2024 21:53
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

👍 thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants