-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add support for Nirmata,cloud-native application management #298
Conversation
As noted on the other comment, we'd like to see the "Overview" section of the PR description filled out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've left some suggestions, please let us know if you have any questions! 😄
plugins/nirmata/api_token.go
Outdated
if section.HasKey("address") && section.Key("address").Value() != "" { | ||
fields[fieldname.Address] = section.Key("address").Value() | ||
} | ||
if section.HasKey("email") && section.Key("email").Value() != "" { | ||
fields[fieldname.Email] = section.Key("email").Value() | ||
} | ||
if section.HasKey("token") && section.Key("token").Value() != "" { | ||
fields[fieldname.Token] = section.Key("token").Value() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there is a reference to fieldname.URL
in the default envvar mapping, but we never import this field.
Let's make sure that our import candidates comply with the structure that the provider expects. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've done the change
needsauth.NotForHelpOrVersion(), | ||
needsauth.NotWithoutArgs(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's opt out of authentication for nctl login
as well. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess getit!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'm not familiar with the Nirmata CLI, are there any other commands which do not require authentication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe version
does not require authentication, but could you confirm the rest, @itsCheithanya?
plugins/nirmata/api_token.go
Outdated
var defaultEnvVarMapping = map[string]sdk.FieldName{ | ||
"NIRMATA_TOKEN": fieldname.Token, | ||
"NIRMATA_URL": fieldname.URL, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we import and require an email, but only provision the token and URL. Is the email something that is required as well?
Otherwise, let's consider making it optional, or, alternatively, omitting it from the importer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Moving in the right direction, let's go 🚀
Name: fieldname.Email, | ||
MarkdownDescription: "Email address registered in Nirmata.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the email required in the item, since it's not provisioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it should be used for this command nctl login
which prompts for all these fields including email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this shell plugin, the user should no longer have to run nctl login
. From what I can find in the documentation (Alternatively, you can use global flags or set the NIRMATA_TOKEN and NIRMATA_URL environment variables.
), this is an alternative to the email-based authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay in that way makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we now make the URL field required and not optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since the docs state that the URL has a default:
--url string the URL address for Nirmata (env NIRMATA_URL) (default "https://nirmata.io")
so not provisioning it would not break the users' workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh okay so should I only import token from the config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay to also import url from the config file. At provisioning time, if there's nothing to provision for the url, then we can use the default value
plugins/nirmata/api_token.go
Outdated
DocsURL: sdk.URL("https://nirmata.com/docs/api_token"), // TODO: Replace with actual URL | ||
ManagementURL: sdk.URL("https://console.nirmata.com/user/security/tokens"), // TODO: Replace with actual URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These URLs don't work for me 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh yeahh I should change it,I didn't find proper documentation about the CLI ,this is the only where it is mentioned
https://downloads.nirmata.io/nctl/downloads/
plugins/nirmata/api_token.go
Outdated
if section.HasKey("email") && section.Key("email").Value() != "" { | ||
fields[fieldname.Email] = section.Key("email").Value() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to import this, since it's not provisioned to the commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yess i feel you are correct ill try without email
needsauth.NotForHelpOrVersion(), | ||
needsauth.NotWithoutArgs(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'm not familiar with the Nirmata CLI, are there any other commands which do not require authentication?
Name: "nirmata", | ||
Platform: schema.PlatformInfo{ | ||
Name: "Nirmata", | ||
Homepage: sdk.URL("https://nirmata.com"), // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the TODO from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsCheithanya This does not seem to be removed yet, could we make sure to remove it, please?
Name: "nirmata", | ||
Platform: schema.PlatformInfo{ | ||
Name: "Nirmata", | ||
Homepage: sdk.URL("https://nirmata.com"), // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the TODO from here.
plugins/nirmata/api_token_test.go
Outdated
func TestAPITokenImporter(t *testing.T) { | ||
plugintest.TestImporter(t, APIToken().Importer, map[string]plugintest.ImportCase{ | ||
"environment": { | ||
Environment: map[string]string{ // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the TODO from here.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just had a chance to review the code, and it looks like a good start, @itsCheithanya. Thank you. A lot of functional testing will be required though and for us to do it, could you help us with some details?
- What is Nirmata?
- How does nirmata.com differ from nirmata.io?
- From this page, nctl and nadm are different CLI. Would you know what each does?
- And from this page, Nirmata Enterprise for Kyverno and Nirmata Policy Manager are two different things as well. Which of those two platforms is the proposed shell plugin targeting?
Lastly but not the least, could you share some screenshots of the shell plugin in action? It'd help a ton with the reviews.
I usually recommend sharing some screenshots/videos to back up the proposed changes in any PR, thanks Cheithanya!
Name: "nirmata", | ||
Platform: schema.PlatformInfo{ | ||
Name: "Nirmata", | ||
Homepage: sdk.URL("https://nirmata.com"), // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsCheithanya This does not seem to be removed yet, could we make sure to remove it, please?
return schema.Executable{ | ||
Name: "Nirmata CLI", | ||
Runs: []string{"nctl"}, | ||
DocsURL: sdk.URL("https://nirmata.com/docs/cli"), // TODO: Replace with actual URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo as well.
needsauth.NotForHelpOrVersion(), | ||
needsauth.NotWithoutArgs(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe version
does not require authentication, but could you confirm the rest, @itsCheithanya?
if section.HasKey("address") && section.Key("address").Value() != "" { | ||
fields[fieldname.Address] = section.Key("address").Value() | ||
} | ||
// if section.HasKey("email") && section.Key("email").Value() != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's completely remove this block instead of leaving them as commented sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsCheithanya this is still commented
Fields: fields, | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of empty lines in this file as well.
}, | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty line here. Let's remove it.
Name: "nirmata", | ||
Platform: schema.PlatformInfo{ | ||
Name: "Nirmata", | ||
Homepage: sdk.URL("https://nirmata.com"), // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like nirmata.com and nirmata.io are two different products/services, but from the same company? Could you share some specifics of what you know about Nirmata? That'd help us reviewers jumpstart the process, vs having to understand the Nirmata concepts manually. Thanks @itsCheithanya!
This plugin is focussed on Nirmata Policy Manager as it supports api keys with nctl cli |
@itsCheithanya as next steps, it would be awesome if you could share a video of the plugin working as expected. There are also a couple of comments which are pending resolution. |
Hey @itsCheithanya - reaching out again, is there anything that we can help with to move this forward? 🚀 |
Closing because of inactivity - please feel free to re-open the PR if you'd like to pick this back up @itsCheithanya 🚀 |
Overview
Type of change
Related Issue(s)
How To Test
Download nctl CLI from here
Run the command
nctl info
to get
Changelog