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.

Sign up for GitHub

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

Jump to bottom

Fix app template #3

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Fix app template #3

merged 4 commits into from
Oct 1, 2024

Conversation

QuentinBisson
Copy link
Contributor

@QuentinBisson QuentinBisson commented Sep 25, 2024 •
edited
Loading

This PR:

  • deploys the alloy app as a component named observability-gateway on MCs
  • The configuration options are a bit funky because the goal is to use the alloy-app from collections (with the name alloy-gateway) but our internal tooling currently prevents us from deploying an app in a collection with a different name Deploy multiple instance of the same application in collections roadmap#3682 is fixed so using this workaround will allow us to switch from this app to the alloy app without having to change the config when we do the migration.

Checklist

  • Update changelog in CHANGELOG.md.
  • Make sure values.yaml and values.schema.json are valid.

@QuentinBisson QuentinBisson self-assigned this Sep 25, 2024
@QuentinBisson QuentinBisson requested a review from a team as a code owner September 25, 2024 11:52
@QuentinBisson QuentinBisson force-pushed the fix-app-template branch 4 times, most recently from 30ac6ca to 5c1beb2 Compare September 25, 2024 12:43
@QuentinBisson QuentinBisson requested a review from a team September 25, 2024 12:44
@QuentinBisson QuentinBisson force-pushed the fix-app-template branch 10 times, most recently from 110960c to 54b745b Compare September 25, 2024 13:38
@QuantumEnigmaa
Copy link

I thought the idea was to integrate this app in another repo until we could put it back in collections under the alloy-gateway name pointing towards the alloy-app repo ? 🤔

Otherwise I'm also fine with this method

@QuentinBisson
Copy link
Contributor Author

Well yes this is the other repo :D

@QuantumEnigmaa
Copy link

I meant "integrate it in another existing repo" 😅

@QuentinBisson
Copy link
Contributor Author

ah yes but if we did that then we would need to migrate the config :D

QuantumEnigmaa
QuantumEnigmaa approved these changes Sep 26, 2024
View reviewed changes
Copy link

@QuantumEnigmaa QuantumEnigmaa left a comment

Choose a reason for hiding this comment

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

Fine with me

hervenicol
hervenicol reviewed Sep 30, 2024
View reviewed changes
helm/alloy-gateway/templates/_helpers.tpl
Comment on lines -31 to -32
application.giantswarm.io/branch: {{ .Chart.Annotations.branch | replace "#" "-" | replace "/" "-" | replace "." "-" | trunc 63 | trimSuffix "-" | quote }}
application.giantswarm.io/commit: {{ .Chart.Annotations.commit | quote }}
Copy link

Choose a reason for hiding this comment

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

We're removing a lot of stuff from the default template. Isn't it good practice to have them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit and branch? We remove it from everywhere actually 😅

@hervenicol
Copy link

It looks like a bit more than a template fix 😅

It's quite hard to spot the differences between generated files and actual code/config.

I haven't seen any config that would make this app act as a Otel / Prometheus / Loki gateway. Did I miss something?

What happens when this is released? As it's pushed to collections it will be deployed to all CAPI MCs. Will it run or will it fail? Are there some related PRs for config?

Also, could you link the source issue?

@QuentinBisson
Copy link
Contributor Author

QuentinBisson commented Sep 30, 2024 •
edited
Loading

So I forgot to link the main issue giantswarm/roadmap#3568 (comment) ... sorry about this.

This application is deployed via collections so the config (to be) will be defined here https://github.com/giantswarm/shared-configs/blob/main/default/apps/alloy-gateway/configmap-values.yaml.template.

When this app is released it will then be deployed to all CAPI MCs and it will work because I've tested it on grizzly.

Once this is setup, we will be able to configure this properly.

The real changes are in the helm directory, https://github.com/giantswarm/alloy-gateway-app/pull/3/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5 the rest is merely templating like github workflow

@QuentinBisson
Copy link
Contributor Author

@hervenicol let me know if you need more info or we can pair on it

QuentinBisson added a commit to giantswarm/alloy-app that referenced this pull request Sep 30, 2024
@QuentinBisson
Remove push to app collections
4fafc7a 
This PR removes the push to app collections to avoid a conflict with giantswarm/alloy-gateway-app#3
@QuentinBisson QuentinBisson mentioned this pull request Sep 30, 2024
Merged
1 task
QuentinBisson added a commit to giantswarm/alloy-app that referenced this pull request Sep 30, 2024
@QuentinBisson
Remove push to app collections (#54)
cb7af25 
This PR removes the push to app collections to avoid a conflict with giantswarm/alloy-gateway-app#3
@hervenicol
Copy link

All right, by default it's not enabled and won't deploy any configmap or pod.
So it's looking good to me.

hervenicol
hervenicol approved these changes Oct 1, 2024
View reviewed changes
Copy link

@hervenicol hervenicol left a comment

Choose a reason for hiding this comment

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

LGTM

@QuentinBisson
Copy link
Contributor Author

Thanks, sorry about the confusion :(

@QuentinBisson QuentinBisson merged commit 5dfbcc2 into main Oct 1, 2024
4 checks passed
@QuentinBisson QuentinBisson deleted the fix-app-template branch October 1, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hervenicol hervenicol hervenicol approved these changes

@QuantumEnigmaa QuantumEnigmaa QuantumEnigmaa approved these changes

Assignees

@QuentinBisson QuentinBisson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@QuentinBisson @QuantumEnigmaa @hervenicol