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

Oracle netplan v2 migration #6024

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

a-dubs
Copy link
Collaborator

@a-dubs a-dubs commented Feb 17, 2025

Proposed Commit Message

feat(oracle): update Oracle Datasource to use netplan v2

Additional Context

Test Steps

Ran the full integration test suite against Noble using a custom deb created from this feature branch.

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

See my inline comments. I think you added some good code, but much of it can move into tests.

Additionally, I forget, why are we moving to support both v1 and v2 now? Once we move to v2, is there any reason we would need to keep the v1 support around?

@a-dubs
Copy link
Collaborator Author

a-dubs commented Feb 19, 2025

@TheRealFalcon thanks for the feedback! I guess the idea here was that by keeping the old v1 code around, we could do the comparisons on live systems that way if anyone ever finds a weird edge case then it will log a warning and they could file a bug. But honestly, now that you mention it, I do kinda prefer the idea of just having a super comprehensive unit test suite to verify it once and be done and then move on with our lives.

@holmanb thoughts? since this was your idea originally?

@holmanb
Copy link
Member

holmanb commented Feb 20, 2025

@TheRealFalcon thanks for the feedback! I guess the idea here was that by keeping the old v1 code around, we could do the comparisons on live systems that way if anyone ever finds a weird edge case then it will log a warning and they could file a bug. But honestly, now that you mention it, I do kinda prefer the idea of just having a super comprehensive unit test suite to verify it once and be done and then move on with our lives.

@holmanb thoughts? since this was your idea originally?

The only benefit that I see of a runtime check is that it might catch edge cases that couldn't be predicted with a battery of unit tests. I'm fine with just getting some good test coverage instead if that's preferred.

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.

3 participants