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

Adjust spryker to use hostname_aliases for nginx #527

Conversation

kierenevans
Copy link
Contributor

@kierenevans kierenevans commented Jan 14, 2021

  • Set hostname_aliases to be an array of the external hostnames for glue, yves and zed
  • Document the glue URLs in the README
  • Use _base's nginx template now that the hostname_aliases is set up
  • Fix spacing in the istio ingress
  • Use a with in the nginx ingress for _base and spryker
  • Update spryker to use istio.gateways rather than a hardcoded one.
  • Fix spryker ingresses only containing one host "localhost" due to .Values.services.nginx.environment.FPM_HOST
  • Restrict environment variables processed in spryker ingress to ones containing _HOST_ in their name

@kierenevans kierenevans added bug Something isn't working enhancement New feature or request harness-spryker Spryker harness labels Jan 14, 2021
@kierenevans kierenevans added this to the 1.0.0 milestone Jan 14, 2021
@kierenevans kierenevans self-assigned this Jan 14, 2021
@kierenevans kierenevans marked this pull request as ready for review January 21, 2021 08:33
@kierenevans kierenevans marked this pull request as draft January 21, 2021 08:33
Base automatically changed from feature/remove-deprecations to 1.0.x January 22, 2021 09:19
@kierenevans kierenevans force-pushed the feature/move-spryker-to-use-hostname-aliases-for-nginx-and-ingress branch from acb5a0f to c41bc84 Compare January 22, 2021 09:44
@kierenevans kierenevans marked this pull request as ready for review January 22, 2021 09:45
Copy link

@hgajjar hgajjar left a comment

Choose a reason for hiding this comment

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

Looks good to me, just not sure about the changes in src/spryker/helm/app/templates/application/webapp/ingress-istio.yaml

annotations:
{{- range $key, $value := .Values.services.nginx.ingress.annotations }}
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think to moving .Values.services.nginx.ingress.annotations to .Values.ingress.annotations for the 1.0.0 release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess there might be annotations for the istio ingress too, though that can be switched at the values.yaml.twig level

Copy link
Contributor

@andytson-inviqa andytson-inviqa Jan 22, 2021

Choose a reason for hiding this comment

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

istio virtualservice only has labels atm, but I'd apply the annotation capability to both of 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.

How does #538 look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now part of this PR :)

@kierenevans kierenevans force-pushed the feature/move-spryker-to-use-hostname-aliases-for-nginx-and-ingress branch from ae50510 to 744d808 Compare January 22, 2021 14:44
Co-authored-by: andytson-inviqa <[email protected]>
@kierenevans kierenevans enabled auto-merge (squash) January 27, 2021 19:48
@kierenevans kierenevans merged commit b14f792 into 1.0.x Jan 27, 2021
@kierenevans kierenevans deleted the feature/move-spryker-to-use-hostname-aliases-for-nginx-and-ingress branch January 27, 2021 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request harness-spryker Spryker harness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants