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 user-assigned identity example and other small fixes #18

Merged
merged 9 commits into from
Nov 17, 2023

Conversation

adamconnelly
Copy link
Collaborator

@adamconnelly adamconnelly commented Nov 16, 2023

Description of the change

I've fixed the user-assigned identity example to correctly pass the --username flag. I've also included a number of other small tweaks in this PR because they were all very small changes:

  • Make sure the log directory exists in case a VM image is being used that doesn't provision it.
  • Switch to a scale_in block instead of the deprecated scale_in_policy.
  • Allow unattended-upgrade to be disabled.
  • Remove double base64-encoding for the private key in the bastion and user-assigned identity examples.
  • Removed some unnecessary \ characters from log output.

I've also disabled the password auth example from running as a test case because of what looks like a bug in the Terraform provider: hashicorp/terraform-provider-azurerm#19322.

Type of change

  • Bug fix (non-breaking change that fixes an issue);
  • New feature (non-breaking change that adds functionality);
  • Breaking change (fix or feature that would cause existing functionality to not work as expected);
  • Documentation (a documentation or example fix not affecting the infrastructure managed by this module);

Checklists

Development

  • All necessary variables have been defined, with defaults if applicable;
  • The code is formatted properly;

Code review

  • The module version is bumped accordingly;
  • Spacelift tests are passing;
  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached;
  • This pull request is no longer marked as "draft";
  • Reviewers have been assigned;
  • Changes have been reviewed by at least one other engineer;

Ensure the Spacelift log directory exists in case the directory wasn't provisioned on the VM image being used.
Switching to using a `scale_in` block instead of `scale_in_policy` to fix a deprecation warning.
- Allow unattended-upgrade to be disabled to save time on startup if it isn't required.
- Add an `apt-get update` to ensure the package sources are up to date before running the upgrade.
- Default the new variable to `true` to maintain backwards compatibility.
The user-assigned identity example was missing the `--username` parameter to `az login`, causing it to fail. It also missed the logging redirects meaning it would fail silently.
Removing the backslashes from a couple of info messages. They aren't needed, and are just output in the logs, which is confusing.
Remove unnecessary base64 encodes/decodes from the bastion and user-assigned identity examples. It forces users to base64-encode the private key passed in via a TF var, which isn't required and adds confusion.
It looks like there's an issue where the Terraform provider is unable to delete the KeyVault resource too soon after it was created. For now I'm just disabling the failing test case.
@adamconnelly adamconnelly requested review from b4k3r and a team November 16, 2023 13:52
@adamconnelly adamconnelly marked this pull request as ready for review November 16, 2023 13:53
@peterdeme
Copy link
Contributor

Maybe bump the version number is the README example code?

I also noticed that the `required_providers` block was wrong so fixed that.
@adamconnelly
Copy link
Collaborator Author

Maybe bump the version number is the README example code?

Makes perfect sense @peterdeme. Done.

@adamconnelly adamconnelly merged commit f54e130 into main Nov 17, 2023
1 check passed
@adamconnelly adamconnelly deleted the adamc/CU-86933khu7-address-feedback branch November 17, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants