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

feat: switch to upstream chart directly #6

Merged
merged 7 commits into from
Jan 31, 2024
Merged

feat: switch to upstream chart directly #6

merged 7 commits into from
Jan 31, 2024

Conversation

mjnagel
Copy link
Contributor

@mjnagel mjnagel commented Jan 30, 2024

Burn some boats.

@mjnagel mjnagel requested a review from corang January 30, 2024 23:13
@mjnagel mjnagel self-assigned this Jan 30, 2024
@mjnagel
Copy link
Contributor Author

mjnagel commented Jan 30, 2024

This is still a WIP, need to make some of the setup less gross with the config, probably by shoving more of the secret creation to the uds-config chart to ensure it is seamless with empty values.

@mjnagel mjnagel marked this pull request as ready for review January 31, 2024 15:14
@mjnagel
Copy link
Contributor Author

mjnagel commented Jan 31, 2024

This should be ready for a review - interested in feedback on the approach taken for wiring up dependencies. I left this as a pretty minimal approach where the zarf package really won't work on its own outside of the context of a bundle where helm values are exposed. I think there's two other options though:

  • Use "magic secrets" that the package will expect to be created in some other way: I don't love this pattern but it is the route that SWF packages have gone down. It also felt weird when I implemented it at first (see this commit - the secrets felt bloated and wasn't sure how IRSA would work given the need to leave the key values empty, but MM would expect the key to still be present in the secret.
  • Expose zarf vars for all the bits and pieces - this would end up being a lot of vars, but it would make the deployment experience with just zarf work.

I kind of lean towards leaving this as-is. Since it is a UDS package I think we should be comfortable assuming UDS CLI, assuming proper documentation. But would prefer alternative opinions to weigh in. If https://www.github.com/mattermost/mattermost-helm/pull/440 were to get merged upstream it would make me feel more comfortable about the magic secret path since we wouldn't need to set particular keys in given secrets.

@MxNxPx
Copy link

MxNxPx commented Jan 31, 2024

This should be ready for a review - interested in feedback on the approach taken for wiring up dependencies. I left this as a pretty minimal approach where the zarf package really won't work on its own outside of the context of a bundle where helm values are exposed. I think there's two other options though:

* Use "magic secrets" that the package will expect to be created in some other way: I don't love this pattern but it is the route that SWF packages have gone down. It also felt weird when I implemented it at first (see [this commit](https://github.com/defenseunicorns/uds-package-mattermost/tree/d25c6bb156a9724bfd548bbc3b0c23d2b9ebd728) - the secrets felt bloated and wasn't sure how IRSA would work given the need to leave the key values empty, but MM would expect the key to still be present in the secret.

* Expose zarf vars for all the bits and pieces - this would end up being a lot of vars, but it would make the deployment experience with just zarf work.

I kind of lean towards leaving this as-is. Since it is a UDS package I think we should be comfortable assuming UDS CLI, assuming proper documentation. But would prefer alternative opinions to weigh in. If https://www.github.com/mattermost/mattermost-helm/pull/440 were to get merged upstream it would make me feel more comfortable about the magic secret path since we wouldn't need to set particular keys in given secrets.

@mjnagel - i am in agreement with merging this PR as-is and refactoring once the helm chart adopts the envFrom change you proposed.

on another note, do we have a way to influence the hostname portion of the virtual service? for example could we deploy this in a way to get chat.uds.dev rather than mattermost.uds.dev for the endpoint? this could be in a later PR also to not hold up this one. thanks!

@mjnagel
Copy link
Contributor Author

mjnagel commented Jan 31, 2024

@MxNxPx I have no problem switching to chat. as the default, a bit hesitant to expose the subdomain as a var though since we aren't doing that anywhere else (i.e. Gitlab). Technically could point a second virtualservice to the same service and configure global.siteUrl accordingly but I'm not sure that's a great option. Suffice to say - would prefer to leave exposing that as a configuration until later but happy to switch to chat. by default if we want that.

@zachariahmiller
Copy link
Contributor

@MxNxPx I have no problem switching to chat. as the default, a bit hesitant to expose the subdomain as a var though since we aren't doing that anywhere else (i.e. Gitlab). Technically could point a second virtualservice to the same service and configure global.siteUrl accordingly but I'm not sure that's a great option. Suffice to say - would prefer to leave exposing that as a configuration until later but happy to switch to chat. by default if we want that.

How would you feel about templating host in the uds-package cr with a default and then not "exposing" subdomain per se, but someone could purposefully override it in their bundle?

@mjnagel
Copy link
Contributor Author

mjnagel commented Jan 31, 2024

@zachariahmiller yeah definitely can make that a helm value if we want, it would be easy. I can go ahead and make that switch, was just noting that this isn't something we have done on any other UDS packages to date.

Copy link

@mikevanhemert mikevanhemert left a comment

Choose a reason for hiding this comment

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

🚀

@mjnagel mjnagel merged commit a7a3d40 into main Jan 31, 2024
3 checks passed
@mjnagel mjnagel deleted the burn-some-boats branch January 31, 2024 20:22
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.

4 participants