-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
New resource: azurerm_automation_webhook
#13893
Conversation
Add test to check if url's are updated properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new resource @wiktorn - overall looks good but i had some questions/comments i've left inline
} | ||
} | ||
|
||
func resourceAutomationWebhookCommonSchema() map[string]*pluginsdk.Schema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure what we gain from this being a seperate function? could we just inline it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can inline that. I will update PR tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Required: true, | ||
ValidateFunc: validate.RunbookName(), | ||
}, | ||
"run_on": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this
"run_on": { | |
"run_on_worker_group": { |
to be a little more explicit what it means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ForceNew: true, | ||
Computed: true, | ||
Sensitive: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we validate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. What's easy to add is: IsURLWithHTTPorHTTPS
. I don't think that there is any official pattern/domain, that we can validate. IIRC API was accepting some invalid (not pointing to Azure) urls on creation (but not on update...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
/* | ||
// missing workergroup creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there no way to create a worker group yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not exposed in Azure REST API. I guess that this might be due to the fact, that worker group is created when agent is installed and registered in Azure. I assumed, that this is only resource missing in terraform, but as I've just now checked, I think it is better just to remove this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you pass in a fake value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can pass the fake value, the only thing is, that there will be an error returned from REST API. As it is quite specific, I've implemented it right now, as expecting this specific error.
* use `run_on_worker_group` instead of `run_on` * inline schema creation function * enable test for checking `run_on_worker_group` using fake value and expect failure
Is there something left to do on my side to get this PR merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wiktorn! sorry for the delay in looping back here but tests pass and code looks great 🙂 Merging 🙌
This functionality has been released in v2.86.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes: #13796.
I haven't tested and I guess that it doesn't support URI rotation as I described:
I don't know if this is at all possible.
Test results:
I'm still working on the documentation