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

Add support for boundary_self_managed_worker resource. #293

Merged
merged 13 commits into from
Nov 24, 2022

Conversation

grantorchard
Copy link
Contributor

Hey folks, initial cut of a self-managed worker resource.
A couple of things to note:

  1. The test for worker-led auth is commented out as I couldn't find a way to generate a worker token on the fly.
  2. I'm not getting tag values back from resourceSelfManagedWorkerRead when they are set via CLI/worker config, but haven't yet figured out why. Any pointers would be appreciated!

Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks for this change! I think we can fix both of the issues you mentioned.

internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker_test.go Outdated Show resolved Hide resolved
@louisruch louisruch self-requested a review November 3, 2022 20:08
Copy link
Contributor

@louisruch louisruch left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this and great work so far, I left a couple of comments inline. Also please do run make docs or go generate to auto generate to documentation. It would be great if you could also add an example config under examples

internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
internal/provider/resource_self_managed_worker.go Outdated Show resolved Hide resolved
@grantorchard grantorchard requested review from johanbrandhorst and removed request for ddebko November 10, 2022 21:02
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

One small comment, otherwise looks good to me!

internal/provider/worker.go Outdated Show resolved Hide resolved
@grantorchard
Copy link
Contributor Author

All changes are in, I think we are good to go.

internal/provider/worker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

LGTM but it'd be great to have @louisruch's thoughts as well. Could you also coordinate with @prsekar to create an issue for supporting tags later?

Copy link
Contributor

@louisruch louisruch left a comment

Choose a reason for hiding this comment

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

I think this is in a good place to merge, thanks for getting to this!

@johanbrandhorst
Copy link
Contributor

Reopening PR to see if we can trigger the CI job

@johanbrandhorst
Copy link
Contributor

Not sure what's up with Circle but I successfully ran the tests locally so lets merge this :)

@johanbrandhorst johanbrandhorst merged commit cfc57d5 into hashicorp:main Nov 24, 2022
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