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

Remove creationTimestamp from ingress #704

Merged

Conversation

Chemaclass
Copy link
Contributor

🔖 Changes

  • Fix indentation for with .Values.ingress.annotations and tls
  • Removed creationTimestamp: null

@andytson-inviqa
Copy link
Contributor

I don't believe there's any issue here, all changes are where the whitespace is truncated {{-

@andytson-inviqa
Copy link
Contributor

andytson-inviqa commented May 13, 2022

though creationTimestamp you're right to remove (just as status shouldn't be in it)

@Chemaclass
Copy link
Contributor Author

ah, well, it make sense. I apply those indentation fixes because the IDE (PhpStorm) told me that so it looks nicer. Do you want me to rollback those changes? Or is it fine as it is right now? We could remove the creationTimestamp at least in this PR. What do you say, @andytson-inviqa?

@andytson-inviqa
Copy link
Contributor

andytson-inviqa commented May 13, 2022

If you rename the ticket to "Remove creationTimestamp from ingresses", then I'd be happy with the formatting changes still in there, however the same changes we'd also make in the base harness https://github.com/inviqa/harness-base-php/blob/1.4.x/src/_base/helm/app/templates/application/webapp/ingress.yaml

@Chemaclass Chemaclass changed the title Fix indentation for webapp/ingress Remove creationTimestamp from ingress May 13, 2022
@andytson-inviqa
Copy link
Contributor

andytson-inviqa commented May 13, 2022

there's actually a few more places that have creationTimestamp in, so I can create a PR that does all of them

@Chemaclass
Copy link
Contributor Author

Yeah, there are creationTimestamp in Deployment, and StatefulSet, but not more Ingress:
Screenshot 2022-05-13 at 13 44 23
Are we sure we want to remove the creationTimestamp from those other places too?

@andytson-inviqa
Copy link
Contributor

I'll merge this PR and do a separate one of the other places

@andytson-inviqa andytson-inviqa merged commit 07b48e8 into inviqa:1.4.x May 13, 2022
@Chemaclass Chemaclass deleted the feature/improve-webapp-ingress branch May 13, 2022 11:46
@andytson-inviqa
Copy link
Contributor

You're right to bring up the question, as the rest are all in template: fields, it means that they'll all cause a rollout on release even if there's nothing else to change

@andytson-inviqa andytson-inviqa added enhancement New feature or request harness-all Changes required to all harness directories labels May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request harness-all Changes required to all harness directories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants