Skip to content
This repository has been archived by the owner on Jun 28, 2023. It is now read-only.

Adds Contour package #514

Merged
merged 1 commit into from
May 17, 2021
Merged

Conversation

skriss
Copy link
Contributor

@skriss skriss commented May 5, 2021

What this PR does / why we need it

This PR is a WIP that adds a Contour package.

Which issue(s) this PR fixes

N/A

Describe testing done for PR

Basic manifest generation with ytt -f addons/packages/contour/bundle/config/ and deployment in a KinD cluster.

Special notes for your reviewer

This is a WIP, looking to get any initial feedback from both @shyaamsn and TCE folks on how I've approached this. I started from the existing packaging in tkg-extensions and made a number of removals/modifications. A few specific questions I have:

  • tkg-extensions had a values.star that among other things defined validation functions for the values.yaml. I didn't see any *.star files in the TCE repo so I omitted this for now. Question is, does it make sense to bring those over into TCE?
  • Contour takes a YAML config file, which is usually provided via config map. Instead of templating each individual field in that YAML config file like tkg-extensions did, I just pull the entire YAML block from values.yaml into the ConfigMap via overlay. From my perspective this is easier to maintain going forward, and the templating seemed to be just adding an extraneous layer. Thoughts on this approach?
  • tkg-extensions had a few different kapp annotations applied to various objects (the namespace, configmap, and Envoy service IIRC); I removed those because I wasn't sure they were necessary for TCE, but if they are, please let me know.

Does this PR introduce a user-facing change?

TODO

@skriss
Copy link
Contributor Author

skriss commented May 11, 2021

hey folks just a reminder that I'm looking for some feedback on approach here (TCE maintainers and @shyaamsn). I've rebased to get rid of the extraneous commits that came up from main.

@seemiller
Copy link
Contributor

@skriss To answer your questions:

  • .star files - You are correct that TCE does not have any .star files. However, I don't see any problem introducing them.
  • values.yaml - I'm always a proponent of doing the simplest thing that works. We are still figuring out how to go about all this ourselves. I'm ok with the approach that you're taking. If we need to adjust in the future, we will.
  • app annotations - Honestly, I'm not sure about this one.

If you feel the package is ready to be pushed up to Harbor, let me know and I can do that (I'll get the SHA that will address that TODO in main.yaml)

@seemiller seemiller self-assigned this May 12, 2021
@seemiller seemiller added the owner/packages Work executed by a package's maintainer label May 12, 2021
@skriss
Copy link
Contributor Author

skriss commented May 12, 2021

Thanks for the feedback @seemiller! Will review and and update soon.

@skriss
Copy link
Contributor Author

skriss commented May 12, 2021

@seemiller I noticed that the contour-operator add-on uses a Bundle name of contour (https://github.com/vmware-tanzu/tce/blob/main/addons/packages/contour-operator/bundle/.imgpkg/bundle.yml#L4), which is what I'd like to use for contour itself. Is it OK to just change contour-operator's Bundle name to contour-operator? If so, I can put up a PR for it.

@seemiller
Copy link
Contributor

Good catch, yes, that should be changed to contour-operator

@skriss skriss force-pushed the contour-tkg-extensions branch 2 times, most recently from 8b4c366 to 5deec1d Compare May 12, 2021 22:29
@shyaamsn
Copy link
Contributor

LGTM

@skriss
Copy link
Contributor Author

skriss commented May 13, 2021

If you feel the package is ready to be pushed up to Harbor, let me know and I can do that (I'll get the SHA that will address that TODO in main.yaml)

@seemiller I've updated this to our latest Contour patch release version, and I think it's in reasonable shape as a first cut. If the next step is pushing stuff to Harbor, then I think it's OK to go ahead with that. What else do I need to do from here? Also, is there a doc that explains how to do a version bump of Contour for future reference?

@seemiller
Copy link
Contributor

@skriss I'll get it packaged and push it up later this morning.

Funny you should ask about versioning, I'm actively working on some approaches to how TCE will handle versioning - #510. Not much to see there at the moment as I'm still actively writing it up.

@skriss
Copy link
Contributor Author

skriss commented May 17, 2021

Hi @seemiller, just checking in to see if you were able to push this to Harbor?

@seemiller
Copy link
Contributor

@skriss I've pushed this to Harbor. You can update the TODO with the following:
projects.registry.vmware.com/tce/contour@sha256:610344aef2d538d0333731a51823dc4d9b774aa430ac007288f589a6a6f7192a

I'll accept the PR after that. Thanks!

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss force-pushed the contour-tkg-extensions branch from 2a59334 to abd5a0e Compare May 17, 2021 20:03
@skriss
Copy link
Contributor Author

skriss commented May 17, 2021

I've updated the TODO and squashed everything down to one commit.

@seemiller seemiller merged commit c36c2d9 into vmware-tanzu:main May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
owner/packages Work executed by a package's maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants