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

Deploy OS UI (Whisker) with Goldmane #3775

Merged
merged 23 commits into from
Feb 22, 2025
Merged

Conversation

Brian-McM
Copy link
Contributor

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

Copy link
Member

@caseydavenport caseydavenport left a comment

Choose a reason for hiding this comment

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

Very first pass. I think building tooling to more consistently, easily, and reliably construct common Objects is probably a good idea, but I do worry about the work required to roll this out across the board and the likelihood of us ending up needing to maintain multiple different ways of constructing objects as a result. So I'm on the fence about shipping it as part of this PR - let's see what @tmjd and @rene-dekker have to say.

@caseydavenport
Copy link
Member

metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

Status WhiskerStatus `json:"status,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to add container overrides to everything this deploys. I'm OK for that to be a follow-up, but I also think it should be rather trivial to add now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're cool with it I'm going to defer it, I tracked it in a ticket. Not sure if goldmane is going to stay here so kind of want to leave overrides until we sort that out.

@Brian-McM Brian-McM marked this pull request as ready for review February 20, 2025 22:22
@Brian-McM Brian-McM requested a review from a team as a code owner February 20, 2025 22:22
Copy link
Member

@caseydavenport caseydavenport left a comment

Choose a reason for hiding this comment

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

A few more minor naming cleanups, and as discussed we need some UTs to start us off. Otherwise this LGTM

@Brian-McM Brian-McM merged commit 32a250a into tigera:master Feb 22, 2025
5 checks passed
LorcanMcVeigh pushed a commit to LorcanMcVeigh/tigera-operator that referenced this pull request Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants