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

v2 #2

Merged
merged 4 commits into from
May 8, 2024
Merged

v2 #2

merged 4 commits into from
May 8, 2024

Conversation

freemvmt
Copy link
Contributor

@freemvmt freemvmt commented Apr 2, 2024

This PR does the following:

  • updates Node to LTS and all dependencies to their latest versions
  • integrates the vultr-node client into the business logic of this action
  • revamps the code (e.g. async/await instead of promises) and makes the logging a bit more verbose
  • enables us to spin up vultr instances with different OSes (Alpine, Fedora CoreOS, Flatcar Container Linux, Ubuntu)
  • can teardown both instances and DNS records reliably (the old code was failing because we weren't paginating the records in order to filter all of them)

I initially put this logic in planx-new (see theopensystemslab/planx-new#2959) but later realised that was the wrong move, and bundled it over here. This should also mean fewer changes to the github workflows.

Copy link

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Fantastic first PR 🎉

Super well documented and clear - great to see.

Nice find on pagination / cursors - makes total sense as the cause of our issues.

A few nitpicks, and some comments on how to handle types, but zero showstoppers here. Looking forward to trying it out 👍

src/api.ts Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@freemvmt freemvmt left a comment

Choose a reason for hiding this comment

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

Actioned all PR comments. I think this is ready to roll but it could do with a final review!

(in the meantime I've tagged the latest comment as v2.0 in order to do one more round of testing)

package.json Show resolved Hide resolved
@freemvmt freemvmt requested a review from jessicamcinchak May 7, 2024 17:42
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Changes look great! A couple very minor questions/comments on final read through, but no blockers - excited to get v2 merged & in use over in planx-new ✨

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/@vultr/vultr-node.d.ts Show resolved Hide resolved
freemvmt added 3 commits May 8, 2024 14:55
…ve README, add missing types for vultr-node, rebuild dist

accept operating system input by name (os_type) rather than Vultr ID
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