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

Separate config classes #1840

Merged
merged 15 commits into from
Apr 30, 2024
Merged

Separate config classes #1840

merged 15 commits into from
Apr 30, 2024

Conversation

jemrobinson
Copy link
Member

✅ 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).
  • You have marked this pull request as a draft and added '[WIP]' to the title if needed (if you're not yet ready to merge).

⤴️ Summary

Separate out serialisable classes (YAMLSerialisable and AzureSerialisable) and ensure that all config files (config, pulumi and contextsettings) are inheriting from one of these.

🌂 Related issues

n/a

🔬 Tests

Tested locally

@jemrobinson jemrobinson requested a review from a team as a code owner April 26, 2024 15:39
@jemrobinson jemrobinson requested a review from JimMadge April 26, 2024 15:39
@jemrobinson jemrobinson force-pushed the separate-config-classes branch from c4aa817 to 6063e03 Compare April 26, 2024 15:42
@jemrobinson jemrobinson force-pushed the separate-config-classes branch from 6063e03 to 6498f4c Compare April 26, 2024 15:44
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.

Can we make sure we have tests for all of these changes.

Maybe we should adjust the triggers so that tests/linting run on PRs to any branch?

@jemrobinson
Copy link
Member Author

Is this already done? The YAMLSerialisable and AzureSerialisable classes aren't intended to be used standalone, and the classes that inherit from them are already tested.

@JimMadge
Copy link
Member

Is this already done? The YAMLSerialisable and AzureSerialisable classes aren't intended to be used standalone, and the classes that inherit from them are already tested.

No, there are tests for SerialisableConfig which are broken now.

I think we should still write tests with simple examples of how we expect these classes to be used. You could look at the SerialisableConfig tests for examples.

@jemrobinson jemrobinson force-pushed the separate-config-classes branch from 10c82f1 to 59e417e Compare April 29, 2024 09:56
@jemrobinson
Copy link
Member Author

@JimMadge: I've written some tests, but note that I actually don't expect these classes to be used on their own :)

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, a few little things to tidy up.

I've written some tests, but note that I actually don't expect these classes to be used on their own

Yes but I don't think I follow your point. If we were to write an ABC we would still need to test it. I think the model here of making a toy "real class" in the tests is a sensible way to do that and neatly separates testing these classes from our real "real classes".

data_safe_haven/utility/__init__.py Show resolved Hide resolved
tests/context/test_context_settings.py Outdated Show resolved Hide resolved
tests/utility/test_yaml_serialisable_model.py Outdated Show resolved Hide resolved
@jemrobinson jemrobinson merged commit 6097b57 into pulumi_storage Apr 30, 2024
10 checks passed
@jemrobinson jemrobinson deleted the separate-config-classes branch April 30, 2024 09:08
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