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.

Sign up for GitHub

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

Jump to bottom

Implement destroy delay #419

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Implement destroy delay #419

merged 1 commit into from
Mar 8, 2021

Conversation

0x2b3bfa0
Copy link
Member

This pull request allows runners to wait the given number of seconds before starting the resource destroy process, so the Terraform provider has enough time to collect the logs in case of failure.

Fixes #413.

@0x2b3bfa0 0x2b3bfa0 requested a review from DavidGOrtega March 1, 2021 13:18
Base automatically changed from release/v0.2.23 to master March 2, 2021 12:43
@0x2b3bfa0
Copy link
Member Author

/tests

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Mar 5, 2021 •
edited
Loading

I thought the same however:

  • I don't think that having an input is a good idea. End user does not need to know nothing about that internal. ENV var can be present for us to test and try.
  • 2min is too long I think (My fault). Specially being so close to the graceful shutdown. I think that 30 secs suffice.

@0x2b3bfa0
Copy link
Member Author

I don't think that having an input is a good idea. End user does not need to know nothing about that internal. ENV var can be present for us to test and try.

Agreed

2min is too long I think (My fault). Specially being so close to the graceful shutdown. I think that 30 secs suffice.

As far as I can tell, this time would add to the graceful shoutdown time, so that shouldn't pose any issue. However, we can try with 30 seconds if you want. The main issue I see with arbitrary delays versus event-driven destroy is that we risk making a wrong estimation.

@DavidGOrtega
Copy link
Contributor

The main issue I see with arbitrary delays versus event-driven destroy is that we risk making a wrong estimation.

In reality the controlled delay is not new a very safe. We are already destroying based on an event. The only thing is that we are delaying it to be able to warn any watcher. It's like our graceful shutdown. I would say that 30 secs is even more than needed.

@DavidGOrtega
Copy link
Contributor

Is this ready?

0x2b3bfa0
0x2b3bfa0 commented Mar 6, 2021
View reviewed changes
bin/cml-runner.js Outdated Show resolved Hide resolved
@0x2b3bfa0 0x2b3bfa0 changed the base branch from master to release/v0.3.1 March 6, 2021 12:27
@restyled-io restyled-io bot mentioned this pull request Mar 6, 2021
Closed
@0x2b3bfa0 0x2b3bfa0 force-pushed the destroy-delay branch 2 times, most recently from 7557d10 to 7848d0f Compare March 6, 2021 12:42
@restyled-io restyled-io bot mentioned this pull request Mar 6, 2021
Closed
@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 6, 2021 •
edited
Loading

@DavidGOrtega, yes, as soon as you consider it ready: all your suggestions have been applied.

DavidGOrtega
DavidGOrtega approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@0x2b3bfa0 0x2b3bfa0 merged commit f7cbbdc into release/v0.3.1 Mar 8, 2021
@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Mar 8, 2021
Merged
@0x2b3bfa0 0x2b3bfa0 deleted the destroy-delay branch March 8, 2021 16:04
@0x2b3bfa0 0x2b3bfa0 added this to the 0.3.1 milestone Mar 8, 2021
DavidGOrtega added a commit that referenced this pull request Mar 18, 2021
@0x2b3bfa0 @github-actions
@restyled-io @restyled-commits @DavidGOrtega
Release 0.3.1 (#419) (#422) (#425) (#426) (#427) (#441) (#445) (#448) (…
81e8a36 
…#431)

* [create-pull-request] automated change (#425)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update dependacron pull request step (#426)

This commit updates the major version for the peter-evans/create-pull-request GitHub Action step.

Required in order to avoid a fatal error caused by the  hard deprecation of the `set-env` and `add-path` standard output commands after CVE-2020-15228.

* Add sanity check for cml-publish with filesystem paths (#427)

* Add sanity check for cml-publish files

Closes #308 and probably also closes #401

* Restyled by prettier (#428)

Co-authored-by: Restyled.io <[email protected]>

* Add tests for cml-publish file errors

* Also removes an stray leading dot on the introduced error message

* Embrace native file error messages for cml-publish

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>

* Add destroy timeout feature (#419)

* [create-pull-request] automated change (#441)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Helio Machado <[email protected]>

* Add support for singleton runners (#422)

* Add support for singleton runners

The --reuse-existing flag will cause cml-runner
to look up for already existing runners with the
specified --labels and skip creating a new one
if that's the case.

* Deprecate --name with a warning

* Apply pre-review suggestions and fixes

* Comment out deprecation warning
* Remove unnecessary comparison
* Rename reuse-existing to reuse
* Fix misconception about empty array truthiness
* Simplify reuse by name logic
* Add missing awaits
* Give precedence to name deduplication over label deduplication
* Remove commented code

* Startup script (#445)

* Startup script

* lint

* snapshots

* Add debugging message for #443 (#448)

This commit doesn't completely fix #443, but closes it until we can reproduce again the error.

* Bitbucket-1000-handling

* bump version

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: DavidGOrtega <[email protected]>
This was referenced Mar 9, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidGOrtega DavidGOrtega DavidGOrtega approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cml-runner - One shot delayed destroy below a graceful shutdown
2 participants
@0x2b3bfa0 @DavidGOrtega