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

Delete action for Container Repository Images and Vault Secrets #7649

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

petrovic-d
Copy link
Collaborator

Action for deleting Container Repository Images and Vault Secrets.
Since there is no API to instantly delete Vault Secrets, the deletion is scheduled for tomorrow. Also Cloud Asset view now shows if Vault Secret is in state: PENDING_DELETION

@jhorvath jhorvath added LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests enterprise [ci] enable enterprise job labels Aug 8, 2024
@jhorvath jhorvath added this to the NB24 milestone Aug 8, 2024
@jhorvath jhorvath self-requested a review August 8, 2024 16:53
@@ -101,6 +103,11 @@ public TreeItemData createDecorations(Node n, boolean expanded) {
d.addContextValues(CTXVALUE_CAP_REFERENCE_NAME);
set = true;
}

if (item instanceof SecretItem) {
Copy link
Member

Choose a reason for hiding this comment

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

@jhorvath these interface-dependent extractions ("if X in Lookup, use put "prefix:${X.property}" into contextValue) seem rather common / useful ... if the usage will grow, we could invent a declarative support for that, would be then far easier to export state to LSP clients.

@jhorvath
Copy link
Contributor

jhorvath commented Aug 9, 2024

The DeleteNodeAction should not be implemented at all. There is already support for deletion. The context value cap:delete is added when Node.canDestroy() returns true in DefaultDecorationsImpl. Additionally, destroy() is called in LspTreeViewServiceImpl for the delete command, which should appear automatically based on the context value.

@petrovic-d petrovic-d force-pushed the delete-cloud-assets-command branch 2 times, most recently from 9804941 to 3b916c4 Compare August 12, 2024 08:09
@petrovic-d petrovic-d requested a review from sdedic August 15, 2024 08:48
@petrovic-d petrovic-d force-pushed the delete-cloud-assets-command branch from 3b916c4 to 086ad44 Compare August 15, 2024 17:38
@petrovic-d petrovic-d requested a review from sdedic August 15, 2024 17:38
Copy link
Contributor

@jhorvath jhorvath left a comment

Choose a reason for hiding this comment

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

Looks good


@Override
public void destroy() throws IOException {
ArtifactsClient client = OCIManager.getDefault().getActiveSession().newClient(ArtifactsClient.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lengthy operation cannot be executed inside destroy(). You have use RequestProcessor here

"DeleteNodeAction=Delete item",
"MSG_ConfirmDeleteAction=Are you sure that you want to delete {0}",
"MSG_DeleteActionFailed=Failed to delete {0}.",
"MSG_DeleteActionSuccess=Successfully deleted {0}."
Copy link
Contributor

Choose a reason for hiding this comment

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

This message is not accurate for scheduled deletions. You should display a different message specifically for scheduled deletions and consider making it more general, as other special cases might arise in the future.


@Override
public void destroy() throws IOException {
RequestProcessor.getDefault().post(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use default RP. Create a dedicated one for this class with throughput 1 like this:
RequestProcessor RP = new RequestProcessor(ContainerTagNode.class);

@jhorvath jhorvath merged commit ee236f0 into apache:master Aug 16, 2024
32 checks passed
@jhorvath jhorvath deleted the delete-cloud-assets-command branch August 16, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enterprise [ci] enable enterprise job LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants