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

Bugfix/namespace length #1257

Merged
merged 5 commits into from
Feb 21, 2024
Merged

Bugfix/namespace length #1257

merged 5 commits into from
Feb 21, 2024

Conversation

michaeljcollinsuk
Copy link
Contributor

@michaeljcollinsuk michaeljcollinsuk commented Feb 20, 2024

📝 Summary

This PR resolves a bug where our expected namespace is too long to be valid for Cloud Platform

Instead of assuming the namespace to be in the format of data-platform-app-<repo-name>, the user will enter their chosen namespace. This is then used when we create the trust policy for the App iam role.

The namespace will be stored on the model. For all existing apps, there is a data migration to add these.

Merging this PR will have the following side-effects:

  • Older app roles will have the data-platform-app prefix, others will not

🔍 What should the reviewer concentrate on?

  • Change to the trust policy
  • Migrations order

🧑‍💻 How should the reviewer test these changes?

  • Will be tested in dev after merge

📚 Documentation status

  • No changes to the documentation are required
  • This PR includes all relevant documentation
  • Documentation will be added in the future because ... (see issue #...)

Will be used to store the Cloud Platform namespace name. Adds migrations
that add the field, then a data migration to add values for existing apps,
then finally a third migration to require values when creating a new
object.
Adds the field in the HTML.
Update the OIDC template to use the entered namespace when
creating the app IAM role.
Updates the create app role task handler to pass a github api
token, so this can be used to create the APP_ROLE_ARN secret as
soon as the App role is created. This is required for apps that
will be deployed without Auth0 clients.
Copy link
Contributor

@jamesstottmoj jamesstottmoj left a comment

Choose a reason for hiding this comment

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

Approved

@michaeljcollinsuk michaeljcollinsuk merged commit c5e9669 into main Feb 21, 2024
5 checks passed
@michaeljcollinsuk michaeljcollinsuk deleted the bugfix/namespace-length branch February 21, 2024 11:42
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