-
Notifications
You must be signed in to change notification settings - Fork 15
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
Use SHM name instead of description for Entra app #2243
Use SHM name instead of description for Entra app #2243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice catch.
N.B. Should this target 5.0.1-rc1 instead of develop? Sounds like a bugfix we will want in the patch release. |
Could do; but depends if we're merging develop into 5.0.1-rc1, as this is up-to-date with develop rather than based on 5.0.1-rc1 |
@craddm : Sorry, for this to work you should probably grab that single commit and apply it on top of |
65ee54e
to
d7da700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check for other places where the description is used?
It isn't ideal that it lives in the context but I think we should ensure changing it doesn't produce any destructive operations.
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
I haven't spotted anywhere else - every other use I've found is just for information (e.g. printing messages to console) |
1c42202
into
alan-turing-institute:release-v5.0.1rc1
✅ Checklist
Enable foobar integration
rather than515 foobar
).develop
.🚦 Depends on
Uses the SHM/context name as part of the name of the Entra application, instead of the description.
This ensures the application will have a consistent name.
🌂 Related issues
Closes #2242
🔬 Tests
Tested locally