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

chore: run go test in ci workflow #27

Merged
merged 3 commits into from
May 10, 2023

Conversation

eddycharly
Copy link
Contributor

This PR runs go test in CI workflow.

Signed-off-by: Charles-Edouard Brétéché <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2023
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 10, 2023
@eddycharly
Copy link
Contributor Author

Anything I should do to avoid this ?
image

@apelisse
Copy link
Contributor

You'd need to be an org member or something? I can look that up right after.

@eddycharly
Copy link
Contributor Author

I think i'm a k/k member, maybe it's not enough ?

@eddycharly
Copy link
Contributor Author

Now test fail 🤔

@apelisse
Copy link
Contributor

That seems completely unrelated? is that a flake?

@eddycharly
Copy link
Contributor Author

Not failing locally, checking.

@eddycharly
Copy link
Contributor Author

Ahah, i had a cluster running locally, once i deleted it test started to fail.

@apelisse
Copy link
Contributor

Ohhh, that's fishy, the tests should definitely not depend on your running environment ...

@eddycharly
Copy link
Contributor Author

eddycharly commented May 10, 2023

Ohhh, that's fishy, the tests should definitely not depend on your running environment ...

  openapiclient.NewLocalFiles(c.localFilesDir), // satisfy expectation users' expectation that provided local files are used
  openapiclient.NewLocalCRDFiles(c.localFilesDir),

Is this expected that we use the same dir for those two clients in the chain ?
First one is supposed to load openapi schema files and second is supposed to load CRD definitions.

@apelisse
Copy link
Contributor

Yeah, I think it's loading all the files and then decides if they are openapi or CRDs (from which we extract the openapi). That should be fine, WDYT?

@eddycharly
Copy link
Contributor Author

eddycharly commented May 10, 2023

My 2 cents is we could provide two flags, nothing prevents those two flags to point at the same dir.
It will allow using different folders.

@eddycharly
Copy link
Contributor Author

And if I apply the fix in #26 test panics 🤔

@apelisse
Copy link
Contributor

apelisse commented May 10, 2023

My 2 cents is we could provide two flags, nothing prevents those two flags to point at the same dir.
It will allow using different folders.

Go for it :-)

@apelisse
Copy link
Contributor

Let's see if @alexzielenski can help?

@eddycharly
Copy link
Contributor Author

Sure, from what I understand, tests are going to depend on the kubernetes version flag ? (because validation and message can vary slightly)

Signed-off-by: Charles-Edouard Brétéché <[email protected]>
@apelisse
Copy link
Contributor

Anything I should do to avoid this ? image

Your name shows "contributor", not "member", that's probably why. If you're a k/k member, you almost certainly can be a kubernetes-sigs member too, especially with the help you give us here.

@alexzielenski
Copy link
Contributor

Since the tests are making use of the RootCommand they get openapiclient.NewKubeConfig(c.kubeConfigOverrides), added automatically. We should add a way to disable that in tests to avoid running clusters being used by tests.

Signed-off-by: Charles-Edouard Brétéché <[email protected]>
@alexzielenski
Copy link
Contributor

/label tide/merge-method-squash
/approve
/lgtm

I am curious whether #17 and #22 together will cause the GitHub test to fail in CI.

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 10, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, eddycharly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7375c61 into kubernetes-sigs:main May 10, 2023
@apelisse apelisse mentioned this pull request May 10, 2023
eyarz pushed a commit to eyarz/kubectl-validate that referenced this pull request Jul 5, 2023
* chore: run go test in ci workflow

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

* fix test

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

* revert test case changes

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

---------

Signed-off-by: Charles-Edouard Brétéché <[email protected]>
alexzielenski pushed a commit to alexzielenski/kubectl-validate that referenced this pull request Jan 2, 2024
* chore: run go test in ci workflow

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

* fix test

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

* revert test case changes

Signed-off-by: Charles-Edouard Brétéché <[email protected]>

---------

Signed-off-by: Charles-Edouard Brétéché <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants