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

style(cdk): rename lifecycle event cdk-cleanup #1092

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

afiune
Copy link
Contributor

@afiune afiune commented Jan 3, 2023

User Story

As a Lacework CDK developer,
I need a way to run a cleanup event for when my component is being uninstalled,
So that I can remove any file, cache, libraries, etc. before the component is removed.

Summary

When installing new CDK components, users use the command lacework component install <component> which runs the lifecycle event cdk-init, this event helps components to deploy any necessary file, cache, config, libraries, etc. during installation. When users want to uninstall the component, we should have a cleanup event to remove anything deployed.

Impact

There are no components that implement this lifecycle so there should not be any impact.

Issue

Jira: https://lacework.atlassian.net/browse/GROW-1081
Documentation: https://lacework.atlassian.net/l/cp/u18EHxx1

Signed-off-by: Salim Afiune Maya [email protected]

@slshen
Copy link
Contributor

slshen commented Jan 4, 2023

The rename itself is fine.

This is the first time I'm looking at the lifecycle facility. So the rest of my comments aren't really related to this PR exactly.

I'd prefer to see these things grouped under a single command i.e instead of running ... cdk-init it runs ... cdk init. Just a preference.

Of more concern is that if ... cdk-init fails there's no feedback to the user. (Unless they run with --debug I guess.)

The simplest thing to do would be to run cdk-init without redirecting stdout/stderr. You wouldn't be able to do the spinner, but I think the overall experience would be better.

I'd also suggest the manifest indicate whether or not cdk-init/cdk-remove is supported.

For that matter there seems to be some overlap between breadcrumbs and cdk-init.

@slshen
Copy link
Contributor

slshen commented Jan 4, 2023

And maybe you've already implemented it, but without a way to do lacework components install --dev manifest.json or equivalent it's really difficult to test an implementation of cdk-init.

I know about the .dev manifest and I'm using that (and I think I could test cdk-cleanup with it.). But I don't think I could test cdk-init. Happy to be wrong about this.

Copy link

@declanwilson-lw declanwilson-lw left a comment

Choose a reason for hiding this comment

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

lgtm
thank you for the docs on this! awesome sauce

As a Lacework CDK developer,
I need a way to run cleanup event for my component,
So that I can remove any file, cache, libraries, etc. during component removal.

**Description**

When installing new CDK components, users use the command `lacework component install <component>`
which runs the lifecycle event `cdk-init`, this event helps components to deploy any necessary
file, cache, config, libraries, etc. during installation. When users want to uninstall the
component, we should have a cleanup event to remove anything deployed.

Jira: https://lacework.atlassian.net/browse/GROW-1081

Signed-off-by: Salim Afiune Maya <[email protected]>
@afiune afiune force-pushed the afiune/GROW-1081 branch 2 times, most recently from ba23fe7 to 5861bfb Compare January 6, 2023 00:24
@afiune afiune changed the title style(cdk): rename lifecycle event cdk-cleanup feat(cdk): Deploy component scaffolding Jan 6, 2023
@afiune afiune changed the title feat(cdk): Deploy component scaffolding style(cdk): rename lifecycle event cdk-cleanup Jan 6, 2023
@afiune
Copy link
Contributor Author

afiune commented Jan 6, 2023

@slshen Thank you for your feedback. My take aways from your comments are:

  • Control lifecycle events via component specification
  • Run only supported events, and if the event fails, error out to the end user
  • How developers can test lifecycle events?
  • Nitpick/Preference: Use nested command <component_binary> cdk <event_name>

I am going to merge this PR and work on some of this suggestions on further PRs,
today I have one more improvement that would be great for you to see and provide
feedback: #1098

This PR will help devs have the events already defined.

@afiune afiune merged commit 4969c91 into main Jan 6, 2023
@afiune afiune deleted the afiune/GROW-1081 branch January 6, 2023 19:22
@lacework-releng lacework-releng mentioned this pull request Jan 6, 2023
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