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

Nginx conf parameters as env var #2203

Merged
merged 7 commits into from
Dec 28, 2023
Merged

Nginx conf parameters as env var #2203

merged 7 commits into from
Dec 28, 2023

Conversation

obermeier
Copy link
Member

Convert example from #2199 to PR.

PR introduces (a) breaking change: no

PR introduces (a) deprecation: no

@github-actions github-actions bot added the ui Anything that affects the UI label Nov 18, 2023
bossenti
bossenti reviewed Nov 20, 2023
View reviewed changes
Copy link
Contributor

@bossenti bossenti left a comment •
edited by obermeier
Loading

Choose a reason for hiding this comment

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

Thanks a lot for this PR @obermeier!

I think we should also set SP_HTTP_SERVER_ADAPTER_ENDPOINT in our Helm chart 🙂

@dominikriemer
Copy link
Member

dominikriemer commented Nov 20, 2023 •
edited by obermeier
Loading

What happens if the variable is not provided?
Will nginx fail to start or just use the empty route?

For the helm charts, we can probably just add a route to the ingress and ingressroutes since we don't use the proxy definitions from the conf file in the k8s setup.

@obermeier
Copy link
Member Author

obermeier commented Nov 20, 2023 •
edited
Loading

What happens if the variable is not provided?

It uses the default.conf without modifictions [Link](https://github.com/apache/streampipes
/blob/405a92950b5a21c6eca3ae4b5cd7cc302222e71b/ui/docker-entrypoint.sh#L22C11-L22C11)

Will nginx fail to start or just use the empty route?

It uses extensions-all-jvm:8090 Link from default.conf.

For the helm charts, we can probably just add a route to the ingress and ingressroutes since we don't use the proxy definitions from the conf file in the k8s setup.

I like the idea

@obermeier
Copy link
Member Author

Thanks a lot for this PR @obermeier!

I think we should also set SP_HTTP_SERVER_ADAPTER_ENDPOINT in our Helm chart 🙂

And the installer/cli scripts

@github-actions github-actions bot added installer Affects the StreamPipes installer documentation Everything related to documentation helm Affects the StreamPipes helm chart labels Nov 27, 2023
@obermeier
Copy link
Member Author

I added the implementations for the installers.

What would you propose in case of a migration? (The case if a configuration exists)

@bossenti bossenti linked an issue Dec 18, 2023 that may be closed by this pull request
Nginx configuration parameters as environment variables #2199
Closed
@obermeier
Copy link
Member Author

obermeier commented Dec 19, 2023 •
edited
Loading

I added the implementations for the installers.

What would you propose in case of a migration? (The case if a configuration exists)

It creates now a backup of the old configuration and applies the new configuration.
Is this solution ok?

@bossenti
Copy link
Contributor

@tenthe @dominikriemer Can you have a look here?
I can't assess the changes

@dominikriemer
Copy link
Member

Hi @obermeier sorry this took me so long.
I think the changes look ok, the only question I have is in the k8s docs - are we re-adding the consul configs in this PR?

@obermeier
Copy link
Member Author

Hi @dominikriemer, thank you so much for your review. I realized there were some errors in the process of merging with the dev branch. I've now pushed an updated version of the k8s documentation.

dominikriemer
dominikriemer reviewed Dec 22, 2023
View reviewed changes
installer/k8s/README.md
| external.couchdb.persistence.storageSize | Size of the CouchDB PV | "1Gi" |
| external.couchdb.persistence.claimName | Name of the CouchDB PersistentVolumeClaim | "couchdb-pvc" |
| external.couchdb.persistence.pvName | Name of the CouchDB PersistentVolume | "couchdb-pv" |

Copy link
Member

Choose a reason for hiding this comment

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

Looks much better now, but does this delete the couchdb parameters? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it exits twice in this README.md. For this reason, I removed one version.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thank you!

@dominikriemer dominikriemer merged commit 41df8e1 into apache:dev Dec 28, 2023
18 checks passed
@IsaakKrut
Copy link
Contributor

@obermeier @dominikriemer change in installer/cli/deploy/standalone/extensions-all-jvm/docker-compose.yml broke local docker-compose startup:
\installer\cli\deploy\standalone\extensions-all-jvm\docker-compose.yml: yaml: unmarshal errors:
line 24: mapping key "environment" already defined at line 20

If you could add the new env variable under the existing environment

@obermeier obermeier mentioned this pull request Dec 28, 2023
Merged
@obermeier
Copy link
Member Author

obermeier commented Dec 28, 2023 •
edited
Loading

Hi @IsaakKrut thank you very much for this hint!
I created a PR 2378.

@obermeier obermeier mentioned this pull request Mar 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bossenti bossenti bossenti left review comments

@dominikriemer dominikriemer dominikriemer approved these changes

Assignees
No one assigned
Labels
documentation Everything related to documentation helm Affects the StreamPipes helm chart installer Affects the StreamPipes installer ui Anything that affects the UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nginx configuration parameters as environment variables
4 participants
@obermeier @dominikriemer @bossenti @IsaakKrut