-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: Support Kubernetes v1.24. Fixes #8320 #9620
Conversation
Will continue the work in #9680 |
We're getting close! |
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Phew. That was epic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM overall. Just a couple of small comments. We should also test this to make sure it still works in older versions of k8s.
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@@ -59,7 +59,7 @@ jobs: | |||
name: E2E Tests | |||
runs-on: ubuntu-latest | |||
timeout-minutes: 25 | |||
needs: [ tests, argoexec-image ] | |||
needs: [ argoexec-image ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argoexec-image
is 3m faster than tests
, so this speeds up the build by 3m
|
||
port=$1 | ||
|
||
lsof -s TCP:LISTEN -i ":$port" | grep -v PID | awk '{print $2}' | xargs -L 1 kill || true | ||
pids=$(lsof -t -s TCP:LISTEN -i ":$port" || true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wrote a lot of errors on linux
M .github/workflows/ci-build.yaml M hack/free-port.sh Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@terrytangyuan green! |
Actually just one last failing test! |
@@ -86,7 +86,19 @@ jobs: | |||
profile: minimal | |||
- test: test-python-sdk | |||
profile: minimal | |||
- test: test-executor | |||
install_k3s_version: v1.21.2+k3s1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is not right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the version were running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be testing 1.24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It defaults to latest (v1.25 ATM). These are for testing backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Can we rename these tests to differentiate them from the existing ones? It might be better to explicitly set the version so that we know what change breaks the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes/no. I wast trying to avoid forcing all the required checks to be changed. That impacts all PRs, which would need to be synced with master.
Using latest reduces maintenance, we don’t need to update it.
|
||
This option is simpler than option 2, as you can combine creating the secret with making it discoverable by name. | ||
|
||
## Option 2 - Discovery By Annotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we going to support both options? I didn't code for it. Will k8s automatically support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only if there are long service account names, or secret already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @sarabala1979 Take another look?
Signed-off-by: Alex Collins <[email protected]> Signed-off-by: Alex Collins <[email protected]> Signed-off-by: juchao <[email protected]>
Signed-off-by: Alex Collins [email protected]
Fixes #8320
Changes: