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

feat: Adds atlas Organization New Parameters Support #1835

Merged
merged 53 commits into from
Jan 18, 2024

Conversation

maastha
Copy link
Collaborator

@maastha maastha commented Jan 12, 2024

Description

  • Adds support for new setting params to organization resource
  • Adds validation to create handler to ensure role_names for new API key contains ORG_OWNER permissions.
  • Updates documentation & example
  • Updates acceptance tests

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-219771

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

main.tf:

resource "mongodbatlas_organization" "test" {
	org_owner_id = "......"
	name = "manual-org-renamed"
	description = "temp desc"
	role_names = ["ORG_OWNER", "ORG_BILLING_ADMIN"]
        multi_factor_auth_required = false
        api_access_list_required = false
        restrict_employee_access = true
	  }
tfstate:

  "resources": [
    {
      "mode": "managed",
      "type": "mongodbatlas_organization",
      "name": "test",
      "provider": "provider[\"registry.terraform.io/mongodb/mongodbatlas\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "api_access_list_required": false,
            "description": "temp desc",
            "federation_settings_id": null,
            "id": "b3JnX2lk:NjVhMWIyZjAwNTJjMzc3ZTY5MDk4Yzlj",
            "multi_factor_auth_required": false,
            "name": "manual-org-renamed",
            "org_id": ".........",
            "org_owner_id": "........",
            "private_key": "..............",
            "public_key": ".........",
            "restrict_employee_access": true,
            "role_names": [
              "ORG_BILLING_ADMIN",
              "ORG_OWNER"
            ]
          },
          "sensitive_attributes": [],
          "private": "bnVsbA=="
        }
      ]
    }
  ]
image

@maastha maastha changed the title feat: new org params feat: Adds atlas Organization New Parameters Support Jan 12, 2024
@maastha maastha marked this pull request as ready for review January 16, 2024 13:17
@maastha maastha requested a review from a team as a code owner January 16, 2024 13:17
@maastha maastha requested a review from Zuhairahmed January 16, 2024 14:51
if err != nil {
if _, _, err := conn.OrganizationsApi.DeleteOrganization(ctx, orgID).Execute(); err != nil {
d.SetId("")
return diag.FromErr(fmt.Errorf("an error occurred when updating Organization settings. Unable to delete organization, there may be dangling resources: %s", err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible here to return both errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated


func isAPIKeyOrgOwner(roles []string) bool {
for _, role := range roles {
if role == "ORG_OWNER" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we have ORG_OWNER as const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

},
}
}

func resourceMongoDBAtlasOrganizationCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
if !isAPIKeyOrgOwner(conversion.ExpandStringList(d.Get("role_names").(*schema.Set).List())) {
return diag.FromErr(fmt.Errorf("`role_names` for new API Key must have the ORG_OWNER role to use this resource"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you could return the err from the method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

}

func TestAccConfigRSOrganization_BasicAccess(t *testing.T) {
acc.SkipTestForCI(t)
Copy link
Member

Choose a reason for hiding this comment

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

is it hard to run it in CI / GH?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry this was missed, this requires setting a up a paying organization in cloud-dev that new organizations can be linked to, I couldn't get it to work with my personal organization in cloud-dev (even though I have billing enabled) so requires looking into setting up payments correctly

Base automatically changed from CLOUDP-219771-update-sdk to master January 17, 2024 21:00
Copy link
Member

@AgustinBettati AgustinBettati left a comment

Choose a reason for hiding this comment

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

LGTM

@maastha maastha merged commit 8b0836f into master Jan 18, 2024
35 checks passed
@maastha maastha deleted the CLOUDP-219771-new-org-params branch January 18, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants