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

Change method of sanitising SRE names #2284

Merged
merged 16 commits into from
Nov 12, 2024

Conversation

craddm
Copy link
Contributor

@craddm craddm commented Nov 8, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

⤴️ Summary

Drops the json_safe function, which is only used to generate SRE filenames without non-alphanumeric characters. The SRE names from the config files are now used, with those names already being sanitised as SafeStrings containing only alphanumeric characters, hyphens, and underscores, all of which can be included in filenames.

This prevents issues with mismatches between the config filename and the SRE name in the config.

🌂 Related issues

Closes #2278

🔬 Tests

Tested locally

@craddm craddm marked this pull request as ready for review November 8, 2024 15:36
@craddm craddm requested a review from a team as a code owner November 8, 2024 15:36
Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

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

LGTM

@JimMadge
Copy link
Member

Can this be a patch release, or will it break existing deployments?

@craddm
Copy link
Contributor Author

craddm commented Nov 11, 2024

I guess it might break them, since the expected config names will now be different (although this should be fixable by reuploading the config).

@jemrobinson
Copy link
Member

If you can write a workaround (download config, upgrade, upload config) then I think this can still be a point release.

Copy link

github-actions bot commented Nov 11, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/config
  sre_config.py
  data_safe_haven/functions
  strings.py
  data_safe_haven/types
  annotated_types.py
  data_safe_haven/validators
  typer.py
  validators.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

Looks good, just want to be careful we aren't reintroducing the same or a similar problem here.

data_safe_haven/config/sre_config.py Outdated Show resolved Hide resolved
Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

Looks good.

I pushed a commit to swap the * in regexes with + so that blank strings are not allowed.
Not sure if it would cause errors but I don't think we expect to allow that.
We could also modify the annotated types to give them a minimum length.

@craddm craddm merged commit 13520c4 into alan-turing-institute:develop Nov 12, 2024
11 checks passed
@craddm craddm deleted the fix-sre-names branch November 13, 2024 14:15
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.

dsh config available is not showing deployed SREs
3 participants