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

Fix: replace the ssh_tunnel connection when the user argument changes #188

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

aidan-melen
Copy link
Contributor

@aidan-melen aidan-melen commented Jun 8, 2023

Attempting to update the ssh connection user in-place with terraform will succeed with the apply, but it will silently failed behind the scenes because the alter connection API only supports rotating the keys. For this reason, we should be replacing the resource when the user changes

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2023

CLA assistant check
All committers have signed the CLA.

force new `ssh_tunnel_connection` resource when `user` changes
@benesch
Copy link
Member

benesch commented Jun 8, 2023

Oof, good spot, thank you! Sorry about this.

@@ -25,6 +25,7 @@ var connectionSshTunnelSchema = map[string]*schema.Schema{
Description: "The user of the SSH tunnel.",
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Member

Choose a reason for hiding this comment

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

We should do this for host and port too, AFAICT. CC @dehume @bobbyiliev

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @aidan-melen. As Nikhil said there are a few attributes that were missing this. Made a pass and think I found the remaining ones #189

@dehume dehume added the bug Something isn't working label Jun 8, 2023
@aidan-melen
Copy link
Contributor Author

At this point, should I just close this PR? looks like this was handled by #189

@benesch
Copy link
Member

benesch commented Jun 8, 2023

All but this one, I think! Thanks again for the help on this.

@benesch benesch merged commit 188e93e into MaterializeInc:main Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants