-
Notifications
You must be signed in to change notification settings - Fork 8
Add password and superuser args to user role resource #736
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
base: main
Are you sure you want to change the base?
Conversation
a7d566b
to
3c0cebd
Compare
3c0cebd
to
3f37ccd
Compare
32ff85d
to
6a7cce5
Compare
pkg/materialize/role.go
Outdated
} | ||
|
||
if b.password != "" { | ||
p = append(p, fmt.Sprintf(` WITH LOGIN PASSWORD %s`, QuoteString(b.password))) |
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.
Open to suggestions here if we should actually make the login
a separate attribute, so instead of:
resource "materialize_role" "admin_user" {
name = "admin"
password = "adminpass"
superuser = true
}
Users would need to explicitly set this, eg:
resource "materialize_role" "admin_user" {
name = "admin"
password = "adminpass"
login = true
superuser = true
}
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 think so. We def have users who use non-login roles to manage grants, so lets make it explicit 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.
Sounds good! Will get this updated.
Tests will be failing until we get the new image with MaterializeInc/materialize#33697 |
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.
Approving because the code seems solid, but I do have a few comments that might be nice to address first.
resource "materialize_role" "admin_user" { | ||
name = "admin_user" | ||
password = var.admin_password | ||
superuser = true | ||
} |
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 this needs login set to true
if b.password != "" { | ||
if b.login { | ||
p = append(p, fmt.Sprintf(` WITH LOGIN PASSWORD %s`, QuoteString(b.password))) | ||
} else { | ||
p = append(p, fmt.Sprintf(` WITH PASSWORD %s`, QuoteString(b.password))) | ||
} | ||
} else if b.login { | ||
p = append(p, ` WITH LOGIN`) | ||
} | ||
|
||
if b.superuserSet { | ||
if b.superuser { | ||
p = append(p, ` SUPERUSER`) | ||
} else { | ||
p = append(p, ` NOSUPERUSER`) | ||
} | ||
} | ||
|
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'm wondering if we should just always provide all WIH options
append(p, ` WITH`)
if b.login {
append(p, ` LOGIN`)
} else {
append(p, ` NOLOGIN`)
}
if b.password {
p = append(p, fmt.Sprintf(` PASSWORD %s`, QuoteString(b.password)))
} else {
p = append(p, ` NOPASSWORD `)
}
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.
Hm yes I like this! Will update the PR now.
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.
@jubrad Actually this might not work and I'm now remembering why I did it that way initially :/
Because in SaaS, where we don't have password enabled, it would make the role resource unusable I think?

077bef1
to
2d67a14
Compare
2d67a14
to
d54762f
Compare
Fixes #730
Blocked on: MaterializeInc/materialize#33697