Skip to content
This repository has been archived by the owner on Sep 16, 2024. It is now read-only.

#459 Implemented DeploySecureCredentialsCommand #460

Merged
merged 1 commit into from
Dec 20, 2022
Merged

#459 Implemented DeploySecureCredentialsCommand #460

merged 1 commit into from
Dec 20, 2022

Conversation

peetkes
Copy link

@peetkes peetkes commented Dec 12, 2022

Added simple version of DeploySecureCredentialsCommand for use with current project at customer that needs to migrate to ml-gradle

Copy link
Contributor

@rjrudin rjrudin left a comment

Choose a reason for hiding this comment

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

Great stuff! Just one change for the test to make it more thorough.


import java.io.File;

public class DeploySecureCredentialsTest extends AbstractAppDeployerTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the manager extends AbstractResourceManager, change this to extend AbstractManageResourceTest. Remove the test method in here and move the sample creds file to src/test/resources/sample-app/src/main/ml-config/security/secure-credentials. And change it to use rest-reader as there's no need for a custom role here.

You can then implement the methods required by the parent class like this:

@Override
protected ResourceManager newResourceManager() {
	return new SecureCredentialsManager(manageClient);
}

@Override
protected Command newCommand() {
	return new DeploySecureCredentialsCommand();
}

@Override
protected String[] getResourceNames() {
	return new String[]{"sec-creds1"};
}

The parent class will do a fairly thorough job of ensuring that create and delete both work.

],
"permission": [
{
"role-name": "sec-cred-role",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use rest-reader here.

@rjrudin rjrudin added this to the 4.4.0 milestone Dec 19, 2022
Copy link
Contributor

@rjrudin rjrudin 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 - can you squash the 4 commits into a single one, and I'll merge it?

@rjrudin rjrudin merged commit 687a89c into dev Dec 20, 2022
@rjrudin rjrudin deleted the issue-459 branch December 20, 2022 20:17
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.

2 participants