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

migrate typha deployment affinities #2598

Merged
merged 1 commit into from
May 1, 2023

Conversation

Tamas-Biro1
Copy link
Contributor

@Tamas-Biro1 Tamas-Biro1 commented Apr 24, 2023

Description

Tigera operator does not support podAffinity and podAntiAffinity during the migration from unmanaged Calico Typha deployment. This PR intended to add this feature.

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.

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2023

CLA assistant check
All committers have signed the CLA.

@Tamas-Biro1 Tamas-Biro1 force-pushed the affinity-migration branch 2 times, most recently from f7517f1 to b4b66af Compare April 25, 2023 11:13
@tmjd
Copy link
Member

tmjd commented Apr 26, 2023

CI hasn't ran and I'm not sure why, so I'm closing and reopening to see if that triggers CI.

Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

Looks like CI is not ran on PRs from external contributors.
I've ran the test target myself and see a failure that I was expecting might be the case.
I'd suggest running go test ./pkg/controller/migration/convert to see the issue.

pkg/controller/migration/convert/core.go Show resolved Hide resolved
@matthewdupre
Copy link
Member

I enabled the CI - FYI @tmjd there should be a prompt next to where the results appear to allow the run. It might be a github permissions issue, although I'd have expected you'd be able to see/approve it too.

Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

I got distracted yesterday and forgot to submit these comments which are fairly trivial. I ran make test since CI isn't running and it all looks good.

pkg/controller/migration/convert/core.go Outdated Show resolved Hide resolved
pkg/controller/migration/convert/core.go Outdated Show resolved Hide resolved
pkg/controller/migration/convert/core.go Outdated Show resolved Hide resolved
@mgleung
Copy link
Contributor

mgleung commented Apr 28, 2023

/sem-approve

@mgleung
Copy link
Contributor

mgleung commented Apr 28, 2023

@Tamas-Biro1 it looks like there's a problem with some of our static checks. Could we ask you to run make gen-files to fix that?

@Tamas-Biro1
Copy link
Contributor Author

@Tamas-Biro1 it looks like there's a problem with some of our static checks. Could we ask you to run make gen-files to fix that?

Running make gen-files does not change anything on the PR to me. What changes should i see?

@mgleung
Copy link
Contributor

mgleung commented Apr 28, 2023

it looks like the issue is in the egress gateways CRD. Can you try make gen-crds? I thought everything should be under gen-files but it seems that CRD generation might not be.

@tmjd
Copy link
Member

tmjd commented May 1, 2023

I've put up #2613 to address the issue @mgleung is talking about. Once it is merged you should be able to rebase your PR and then it will be fixed.

@tmjd
Copy link
Member

tmjd commented May 1, 2023

#2613 has been merged so if you can update your branch it should allow CI to pass.

@Tamas-Biro1
Copy link
Contributor Author

#2613 has been merged so if you can update your branch it should allow CI to pass.

yep, the branch has been rebased. thanks!

@mgleung
Copy link
Contributor

mgleung commented May 1, 2023

/sem-approve

Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

LGTM

@tmjd tmjd merged commit b7c796e into tigera:master May 1, 2023
@tmjd
Copy link
Member

tmjd commented May 1, 2023

Thank you for this change.

@mgleung mgleung mentioned this pull request Jun 1, 2023
5 tasks
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.

6 participants