Closed JefMasereel closed 2 years ago
(for anyone watching, we did a live review) @JefMasereel ping when you're ready for round two cheers!
This fork currently builds and runs without problems, although that still needs to be verified with formal testing. Manual use of the Hetzner resource seems to be stable. However, there are some caveats that might need to be addressed for applications at scale. Cfr code comments.
Open for review, in the meantime I will start working on hetzner_test.go.
(I've also triggered the test run-- I forget sometimes, so ping if you ever want it running again... It should catch some of the formatting/style issues for you automatically.)
I made the requested changes. If everything is alright I will squash it all into a single commit, and then we should be ready to merge. Let's do one more structural review as you suggested?
I pushed some more updates, but I haven't fully addressed some of the open comments yet. To be continued
Yes please =D
On Thu, Jan 27, 2022 at 5:00 PM Joe Groocock @.***> wrote:
@.**** commented on this pull request.
In examples/lang/hetzner0.mcl https://github.com/purpleidea/mgmt/pull/681#discussion_r794033264:
@@ -0,0 +1,40 @@ +import "os" +#import "deploy" + +# read token from a local path outside repo: +$f = "../mgmt-dev/apitoken.txt" +$token = os.readfile($f) + +# alternatively, read from the deploy: +# $token = deploy.readfile(...) + +# or give the string directly: +# $token = "..." + +hetzner "resourcedemo" {
Would it make sense to use a namespaced name for this resource, like hetzner:vm (or whatever nomenclature Hetzner uses for their instances) in case you/someone wants to add other Hetzner resources in the future, to say add SSH keys or other things that Hetzner provide
— Reply to this email directly, view it on GitHub https://github.com/purpleidea/mgmt/pull/681#pullrequestreview-865477399, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABA7M4XP4XFALPA3JOUU5TUYG6ARANCNFSM5H27YCHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
If we really have to maintain consistency across the mgmt language then I think it's also important to consider the same naming problem for other cloud providers that might be added in the future. If we want to add eg. OVH support at some point, maybe it's cleaner to turn the names around and work with server:hcloud, server:robot, server:ovh...?
This way you define the function first, and then the name of (one of) the supported provider(s). Thoughts? I do realize that this implies that the API token for example should be called token:hcloud, and in that case the token specifier might not be super clear to the user...
I disagree, but I've reflected upon the point. If it makes sense to try to unify various different resources, then someone can always make a cloud:vm resource that automatically chooses one or something.
For now it's best IMO that each resource model the thing it's trying to as faithfully as possible to that upstream as long as this model makes sense for our users.
HTH
For Message ID: @.***>
Made some changes based on the received feedback and squashed everything into a single commit. I still have a shortlist of potential additions or changes, but nothing that can't be added as later features. What is in the commit now seems to be ready for use, but please let me know if I missed anything.
Pushed the new updates, I think I covered all your feedback. Please let me know if I missed anything. Playing around with the mcl demo it seems to function as intended, but some automated testing would be helpful to make sure all cases are covered. Hopefully this provides a good enough basis to merge now and then start adding features over time?
Please excuse my horrible delay in doing a final review and merge here! It's been great reading through all of this, and it's obviously much better to get this in and people hacking on it more than to keep it out. Thank you again and looking forward to seeing more hetzner patches in the future!
(I rebased and merged in b26f842de19a31c36fb5149f292b9117dbf4bed7)
\o/
First draft for a new resource supporting hcloud instances (Hetzner). The golang wrapper hcloud-go is used to interact with the Hetzner API. The resource is currently named HetznerRes, but this can be changed if needed.
reference docs
to be discussed