Skip to content

(RE-15124) Implement DNS Plugins and Remove api v1 and v2#551

Merged
yachub merged 19 commits into
mainfrom
vmpooler-dns-gcp
Mar 28, 2023
Merged

(RE-15124) Implement DNS Plugins and Remove api v1 and v2#551
yachub merged 19 commits into
mainfrom
vmpooler-dns-gcp

Conversation

@yachub

@yachub yachub commented Feb 7, 2023

Copy link
Copy Markdown

@e-gris e-gris left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only comments on Ruby style or idiomatic usage. Feel free to take or ignore.

Also, generally, avoid method variables like name, Even the method comments have suggestions for better choices.

I've shown a few examples but didn't flag them all.

Comment thread lib/vmpooler/dns.rb
Comment thread lib/vmpooler/dns.rb Outdated
Comment thread lib/vmpooler/dns.rb Outdated
Comment thread lib/vmpooler/dns/base.rb Outdated
Comment thread lib/vmpooler/dns/base.rb Outdated
Comment thread lib/vmpooler/dns/base.rb
Comment thread lib/vmpooler/pool_manager.rb Outdated
@yachub yachub changed the title (RE-15124) Implement DNS Plugins (RE-15124) Implement DNS Plugins and Remove api v1 Feb 16, 2023
@yachub yachub changed the title (RE-15124) Implement DNS Plugins and Remove api v1 (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 Feb 16, 2023

@e-gris e-gris left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some more purely stylistic/maintainability comments; certainly ignorable for now. I called out just a couple of examples.

Nested 'ifs' and variable mutations are great places for bugs to hide and are often easily refactored into something more stable

Comment thread lib/vmpooler/api/v2.rb
Comment thread lib/vmpooler/api/v2.rb
Comment thread lib/vmpooler/api/v2.rb
@yachub yachub changed the title (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 [WIP] (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 Feb 28, 2023
@yachub yachub changed the title [WIP] (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 Mar 1, 2023
@yachub yachub marked this pull request as ready for review March 1, 2023 18:25
@yachub yachub requested a review from a team as a code owner March 1, 2023 18:25
@yachub yachub changed the title (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 [WIP] (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 Mar 1, 2023

@genebean genebean left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, I am only part of the way through, but it seems that this PR introduces breaking changes within the versioned API endpoint api/v2... that is not okay. Breaking changes need to be implemented as a new api version. I think what you are doing here api wise need to be done as part of api/v3. It is fine if you kill of the v1 and v2 endpoints as part of this work too.

Comment thread README.md Outdated
Comment thread README.md
Comment thread docs/API-v2.md
Comment thread lib/vmpooler/api/reroute.rb Outdated
@yachub yachub force-pushed the vmpooler-dns-gcp branch 4 times, most recently from df62e29 to f0ecbe3 Compare March 14, 2023 11:45
@yachub yachub changed the title [WIP] (RE-15124) Implement DNS Plugins and Remove Deprecated api v1 [WIP] (RE-15124) Implement DNS Plugins and Remove api v1 and v2 Mar 14, 2023

@genebean genebean left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please don't merge until after squashing (I know you are working on it). Adding a label to ensure this.

@yachub

yachub commented Mar 27, 2023

Copy link
Copy Markdown
Author

Just working on squashing commits now

@yachub yachub changed the title (RE-15124) Implement DNS Plugins and Remove api v1 and v2 [WIP] (RE-15124) Implement DNS Plugins and Remove api v1 and v2 Mar 27, 2023
@yachub yachub force-pushed the vmpooler-dns-gcp branch from 7dd62b9 to 8d44e4f Compare March 28, 2023 00:38
@yachub yachub force-pushed the vmpooler-dns-gcp branch from 8d44e4f to 669ecae Compare March 28, 2023 01:30
@yachub yachub changed the title [WIP] (RE-15124) Implement DNS Plugins and Remove api v1 and v2 (RE-15124) Implement DNS Plugins and Remove api v1 and v2 Mar 28, 2023
@yachub yachub added backwards-incompatible This issue or pull request includes breaking changes and removed do not merge labels Mar 28, 2023
@yachub yachub merged commit b6819de into main Mar 28, 2023
@yachub yachub deleted the vmpooler-dns-gcp branch March 28, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards-incompatible This issue or pull request includes breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants