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

Update bundles per arm mixin usage #862

Merged
merged 10 commits into from
Jan 27, 2020
Merged

Conversation

vdice
Copy link
Member

@vdice vdice commented Jan 23, 2020

What does this change

Updates/refactors all example and workshop bundles that had previously used the deprecated azure mixin to use the arm mixin.

In doing so and testing the subsequent bundles, I also sometimes made some tweaks to other aspects of the bundles, for instance, updating terraform and terraform provider versions (and corresponding usage), updating some ARM template values, aligning parameter name formats (usually updating hyphenated params to snake case as the majority dictated), etc.

What issue does it fix

Closes #861

Notes for the reviewer

Please call out any changes/updates that we don't wish to make at this time.

Checklist

  • Unit Tests
  • Documentation
    • Documentation Not Impacted

@vdice vdice changed the title Ref/arm mixin bundles Update bundles per arm mixin usage Jan 23, 2020
@vdice vdice force-pushed the ref/arm-mixin-bundles branch from d8a5753 to a0bf4ed Compare January 23, 2020 19:48
@@ -31,8 +31,8 @@ Once a parameter has been declared in the `porter.yaml`, Porter provides a simpl

```yaml
install:
- description: "Install MySQL"
helm:
- helm:
Copy link
Member

Choose a reason for hiding this comment

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

Dang I thought I had gotten all of these, so glad you noticed! 🙇‍♀

@@ -37,7 +37,7 @@ The bundle will use the service principal created above to interact with Azure.
```

* Update params for your deployment
* change the `invocationImage` Docker repo to match your Docker Hub account (line 4)
* change the `tag` Docker repo to match your Docker Hub account (line 5)
Copy link
Member

Choose a reason for hiding this comment

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

nit/suggestion: leave off the line # as it's easy to drift without us noticing

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call; will remove.

@@ -35,6 +36,15 @@ install:
- "--tenant"
- "{{ bundle.credentials.TENANT_ID}}"

- exec:
Copy link
Member

Choose a reason for hiding this comment

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

@jeremyrickard Now that we have the az mixin, long term (not this PR) do we want to update this to use the az mixin?

mysql_password=iloveporter-127c6!J9$
Copy link
Member

Choose a reason for hiding this comment

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

lol strong stance there 😉 Take that CNAB

Copy link
Member Author

Choose a reason for hiding this comment

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

😂 Also added those extry chars b/c Azure was like 'password not complex enuff'

@@ -67,8 +72,8 @@ install:
externalDatabase.database: "{{ bundle.parameters.database_name }}"

uninstall:
# TODO: enable once the porter-azure mixin supports this action
# - azure:
# TODO: enable once the porter-arm mixin supports this action
Copy link
Member

Choose a reason for hiding this comment

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

arm supports uninstall where the old azure mixin didn't. Maybe a follow-up would be to implement the stanza for uninstall

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice! I'd be down to add it in this same PR, if you are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a link to the issue around implementing uninstall (getporter/arm-mixin#7) in the comment.

Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♀

@@ -1,42 +1,37 @@
resource "random_string" "password" {
length = 16
special = true
override_special = "/@\" "
override_special = "/@£$"
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member Author

@vdice vdice Jan 23, 2020

Choose a reason for hiding this comment

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

😂 the " was messing with my VSCode formatting... all lines below it weren't right... 'twas thinking it was all in one big quote. Obvi more of a terraform extension issue buuut I figured the actual override chars didn't really matter (pulled the new chars from the azurerm docs/examples).

Copy link
Member

Choose a reason for hiding this comment

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

🖖 Carry on!

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM

Not sure if there are any dangling things you wanted in this PR. I'll leave to you to merge when ready

@vdice vdice merged commit ad06cce into getporter:master Jan 27, 2020
@vdice vdice deleted the ref/arm-mixin-bundles branch January 27, 2020 16:34
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.

Refactor: Update applicable bundles to use the arm mixin
2 participants