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

feat: Add pause_after config to powershell provisioner like shell #11792

Merged
merged 2 commits into from
May 30, 2022
Merged

feat: Add pause_after config to powershell provisioner like shell #11792

merged 2 commits into from
May 30, 2022

Conversation

teddylear
Copy link
Contributor

@teddylear teddylear commented May 18, 2022

feat: Add pause_after config to powershell provisioner like shell

Added a simple test to make sure that duration of Provision is longer than pause_after setting.

Closes #10887

@teddylear teddylear changed the title [WIP] feat: Add pause_after config to powershell provisioner like shell feat: Add pause_after config to powershell provisioner like shell May 18, 2022
@teddylear teddylear marked this pull request as ready for review May 18, 2022 21:39
@teddylear teddylear requested a review from a team as a code owner May 18, 2022 21:39
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @teddylear,

The code looks good to me as it stands, thanks for the contribution!

There seems to be a small issue with the pipeline, but it shouldn't be a problem, we'll merge this soon.

Thanks again!

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

So my first review was made before I had run make ci-lint and make fmt-check locally, sorry for this.

Both steps have pointed out some suggestions that I have summarized in my latest review, if you can address those we can merge this.

Not that normally these are run by the CI pipeline, for some reason these have not been triggered on your PR.
If you modify the code further, could you consider running both make ci-lint and make fmt-check so the code is in an optimal state and primed to be merged?

Thanks and sorry for forgetting to check these in my previous review!

provisioner/powershell/provisioner.go Outdated Show resolved Hide resolved
provisioner/powershell/provisioner.go Outdated Show resolved Hide resolved
provisioner/powershell/provisioner_test.go Outdated Show resolved Hide resolved
provisioner/powershell/provisioner_test.go Outdated Show resolved Hide resolved
@teddylear
Copy link
Contributor Author

@lbajolet-hashicorp Thanks for the review! I updated code to pass make fmt-check and make ci-lint. Please let me know if there's anything else I should change.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

With the latest changes, all checks pass; LGTM!

Thanks for the contribution again @teddylear, much appreciated

@lbajolet-hashicorp lbajolet-hashicorp merged commit cbc67b7 into hashicorp:main May 30, 2022
@github-actions
Copy link

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 issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pause_after for provisioner/powershell
2 participants