Skip to content

feat(iam): creation of user as Member #3029

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

giacomosirri
Copy link

This Pull Request implements the feature requested in the issue #3021

@giacomosirri giacomosirri requested review from a team and remyleone as code owners April 7, 2025 08:17
@github-actions github-actions bot added the iam IAM issues, bugs and feature requests label Apr 7, 2025
@remyleone remyleone changed the title feature #3021: (IAM) Creation of user as Member feat(iam): creation of user as Member Apr 7, 2025
@crlptl
Copy link

crlptl commented Apr 7, 2025

Hello @giacomosirri and thank you for this PR.
The need of TF implementation for IAM members is real and we need to work on it ASAP.
We will not accept the contribution as it stands because it would have implications on member feature's future that we see differently - for example we may implement a dedicated resource scaleway_iam_member. Someone from the Scaleway team will work on this resource in the next weeks.

The guest type will disappear in the next months, and the fields you mentioned that cannot be updated will be updatable in very soon too.

However, I would like to thank you for your contribution. Can you give me your Organization-id (here or on Slack Scaleway community - Cyril Scw) so that I can put a voucher on your Organization? Thanks again

@giacomosirri
Copy link
Author

Hello @crlptl,
Thank you very much for the feedback and the kind voucher proposal. I understand what you mean, it is a complex matter that probably requires more careful considerations, so I look forward to see these features fully implemented.

Anyway, the organization id is: daf36079-e52c-416c-9535-d06742e48acc.

Btw, I also tried to access the slack community from the link in the footer of the website, but it appears to be broken:
image

Thank you again.

@remyleone remyleone closed this Apr 7, 2025
@Gnoale
Copy link
Contributor

Gnoale commented Apr 30, 2025

Hello @giacomosirri, after careful consideration it happens that your approach fits well the current API state and is future proof.
I re open your PR and will make a few suggestions if you are still willing to submit it.
Thank you.

@Gnoale Gnoale reopened this Apr 30, 2025
@Gnoale Gnoale force-pushed the member-user-creation branch from cbafdfd to 9fcac44 Compare April 30, 2025 15:17
Copy link
Contributor

@Gnoale Gnoale left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, I made a few suggestion, feel free to tell me if you don't have time to work on this I would then make the changes myself

@@ -140,6 +236,18 @@ func resourceIamUserUpdate(ctx context.Context, d *schema.ResourceData, m interf
return diag.FromErr(err)
}

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this comment it should be now supported

resource.TestCheckResourceAttr("scaleway_iam_user.guest_user", "tags.#", "2"),
resource.TestCheckResourceAttr("scaleway_iam_user.guest_user", "tags.0", "tf_tests"),
resource.TestCheckResourceAttr("scaleway_iam_user.guest_user", "tags.1", "tests"),
// The username is the same as the email for Guest users.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

@giacomosirri
Copy link
Author

Hello @Gnoale, thank you for the update, that's good to hear!
I will take the time to work on it and update the code based on your suggestions. I might be able to submit it within this week, at the latest next week.

@giacomosirri giacomosirri force-pushed the member-user-creation branch from 9fcac44 to ab57ce0 Compare May 14, 2025 15:58
@giacomosirri giacomosirri force-pushed the member-user-creation branch from 3282040 to 344f1e8 Compare May 15, 2025 10:55
@giacomosirri
Copy link
Author

@Gnoale @remyleone hello,
I'm pinging you here just to let you know that I have implemented the requested changes.
The acceptance tests were all ok and as far as I can tell now everything works pretty much as I would expect. However, if you notice something wrong, please let me know so that I can fix it!
Thank you

Copy link
Contributor

@Gnoale Gnoale left a comment

Choose a reason for hiding this comment

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

Hello @giacomosirri thanks for you work, I made another review pass, it is almost good I think

I suggest we remove the ForceNew flag from the "email" field and return an error in case of a guest email update attempt.
Since guest will be deprecated

We could add a hint about this behavior in the documentation too

What do you think @remyleone @crlptl

@@ -23,20 +23,63 @@ func ResourceUser() *schema.Resource {
},
SchemaVersion: 0,
Schema: map[string]*schema.Schema{
"organization_id": account.OrganizationIDOptionalSchema(),
// User input data
"email": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ForceNew

Copy link
Contributor

Choose a reason for hiding this comment

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

Description: "The email of the iam user, in case of a member, this field is not editable",

}, scw.WithContext(ctx))
if err != nil {
return diag.FromErr(err)
}
Copy link
Contributor

@Gnoale Gnoale May 15, 2025

Choose a reason for hiding this comment

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

Suggested change
}
}
if d.HasChange("email") {
return diag.FromErr(fmt.Errorf("The email of a guest user cannot be updated, you need to create a new user"))
}

}
}
} else {
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

rm comment

* For this reason, even though the IAM API supports it, the email is not considered
* an updatable field here.
*/
if d.HasChanges("tags", "first_name", "last_name", "phone_number", "locale") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if d.HasChanges("tags", "first_name", "last_name", "phone_number", "locale") {
if d.HasChanges("tags", "first_name", "last_name", "phone_number", "locale", "email") {

FirstName: scw.StringPtr(d.Get("first_name").(string)),
LastName: scw.StringPtr(d.Get("last_name").(string)),
PhoneNumber: scw.StringPtr(d.Get("phone_number").(string)),
Locale: scw.StringPtr(d.Get("locale").(string)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Locale: scw.StringPtr(d.Get("locale").(string)),
Locale: scw.StringPtr(d.Get("locale").(string)),
Email: scw.StringPtr(d.Get("email").(string)),

{
Config: `
resource "scaleway_iam_user" "member_user" {
email = "[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
email = "foo@scaleway.com"
email = "foobar@scaleway.com"

Check: resource.ComposeTestCheckFunc(
testAccCheckIamUserExists(tt, "scaleway_iam_user.member_user"),
acctest.CheckResourceAttrUUID("scaleway_iam_user.member_user", "id"),
resource.TestCheckResourceAttr("scaleway_iam_user.member_user", "email", "[email protected]"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource.TestCheckResourceAttr("scaleway_iam_user.member_user", "email", "foo@scaleway.com"),
resource.TestCheckResourceAttr("scaleway_iam_user.member_user", "email", "foobar@scaleway.com"),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iam IAM issues, bugs and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants