-
Notifications
You must be signed in to change notification settings - Fork 4
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 Sudo Commands Bundle #131
Add Sudo Commands Bundle #131
Conversation
…hub.com:okta/terraform-provider-oktapam into ulfat/terraformImplementationForSudoOKTA-707627
go.mod
Outdated
|
||
require ( | ||
github.com/atko-pam/pam-sdk-go v1.0.68 | ||
github.com/atko-pam/pam-sdk-go v1.0.70-0.20240605060748-0f7af3861a5e |
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.
we should be using specific tagged releases. If you're waiting for a PR within the SDK to be approved, you'll need to get that done before this goes through.
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.
Yes, this is in pending review, have it here to prove it's tested. I can change it once that's merged.
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.
pam-sdk PR https://github.com/atko-pam/pam-sdk-go/pull/64 and the spec PR https://github.com/atko-pam/platform/pull/8512
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.
.terraformrc
Outdated
@@ -0,0 +1,10 @@ | |||
provider_installation { |
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.
we shouldn't be committing this file
} | ||
|
||
func ListSudoCommandsBundles(ctx context.Context, sdkClient SDKClientWrapper) (*pam.ListSudoCommandBundleResponse, error) { | ||
request := sdkClient.SDKClient.SudoCommandsAPI.ListSudoCommandBundles(ctx, sdkClient.Team) |
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.
do we need to handle pagination 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.
@SajanAlexander-okta - I was looking into adding support for pagination here but I see the generated sudo command bundle request does not have the pagination-related fields like ListSecrets has. Should I manually add it to the sdk or am I missing something?
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 things to check:
- does the API itself support pagination?
- does the open api spec define the pagination fields in the response?
@@ -23,4 +23,5 @@ const ( | |||
LinkSecret = "For details, see [Secrets](https://help.okta.com/okta_help.htm?type=oie&id=ext-pam-secrets)" | |||
LinkSecurityPolicy = "For details, see [Security policy](https://help.okta.com/okta_help.htm?type=oie&id=ext-pam-policy)." | |||
LinkCloudConnection = "For details, see [Cloud Connection](https://help.okta.com/okta_help.htm?type=oie&id=ext-pam-entitlement-aws-connect)." | |||
LinkSudoCommandsBundle = "For details, see Sudo Commands Bundle (TBD)." |
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.
Ideally, we'll want to get the link here before merging.
Computed: true, | ||
Description: descriptions.Name, | ||
}, | ||
attributes.RunAs: { |
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.
all of these should have descriptions. please work with the doc-writer(s) on the right wording for these. these should match / be similar to the docs for the API spec
func readSudoCommandsBundleFromResource(d *schema.ResourceData) (*pam.SudoCommandBundle, diag.Diagnostics) { | ||
var diags diag.Diagnostics | ||
var structuredCommands []pam.SudoCommandBundleStructuredCommandsInner | ||
if scs, ok := d.GetOk(attributes.StructuredCommands); ok { |
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.
since this is a required field, we should have some type of error if ok
is false
Result: &structuredCommandResource, | ||
TagName: "json", | ||
} | ||
decoder, dErr := mapstructure.NewDecoder(cfg) |
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.
interesting - i hadn't used this library before - it could simply some other stuff within the provider.
} | ||
|
||
func resourceSudoCommandsBundleDelete(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { | ||
// there isn't a true delete with password settings, and as databases don't specify ManagedPrivilegedAccounts, |
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 this comment relevant?
resource "oktapam_sudo_commands_bundle" "test_acc_sudo_commands_bundle" { | ||
name = "%s" | ||
structured_commands { | ||
command = "/bin/run.sh" |
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.
there are a lot of other attributes that are on this resource - could we get some better coverage? doesn't have to be everything. also, when adding more attributes, please be sure to check them for their presence and correctness within the tests.
resourceName := "oktapam_sudo_commands_bundle.test_acc_sudo_commands_bundle" | ||
sudoCommandsBundleName := fmt.Sprintf("test-sudo-commands-bundle-%s", randSeq()) | ||
|
||
resource.Test(t, resource.TestCase{ |
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.
we generally try and run both a create and update for resources when doing a test.
Closing this because created pull/132 with verified commmits |
CReate a new provider for sudo commands bundle.
Used https://github.com/atko-pam/pam-sdk-go/pull/64 to generate the client