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

remove unit test for previously removed consul-cni validation #3765

Closed
wants to merge 1 commit into from

Conversation

tgross
Copy link
Member

@tgross tgross commented Mar 15, 2024

In #1527, we added support for OpenShift and Multus, which meant that the consul-cni plugin was no longer necessarily the final CNI plugin run. While working on a patch to allow compatibility with Nomad transparent proxy, I discovered (via git bisect) we'd never removed a now-failing unit test of the plugin for the validation step. It looks like the remaining unit tests still cover the remaining validation, so we can safely remove this test.

Ref: #1527
Ref: hashicorp/nomad#10628

How I've tested this PR

cd ./control-plane/cni; go test .

How I expect reviewers to test this PR

As above

Checklist

In hashicorp#1527, we added support for OpenShift and Multus, which meant that the
`consul-cni` plugin was no longer necessarily the final CNI plugin run. While
working on a patch to allow compatibility with Nomad transparent proxy, I
discovered we'd never removed a now-failing unit test of the plugin for the
validation step. It looks like the remaining unit tests still cover the
remaining validation, so we can safely remove this test.

Ref: hashicorp#1527
Ref: hashicorp/nomad#10628
@tgross
Copy link
Member Author

tgross commented Mar 18, 2024

Thanks @thisisnotashwin! I don't have merge authority on this repo though, so if you're happy with it feel free to merge!

@thisisnotashwin thisisnotashwin added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.2.x This release branch is no longer active. backport/1.3.x This release branch is no longer active. backport/1.4.x labels Mar 18, 2024
@jmurret
Copy link
Member

jmurret commented Mar 18, 2024

hey @tgross, CI won't run on this fork because the branch does not have access to the token required for secrets used in CI. The easiest thing to do since you are in the HashiCorp org is to create a PR from the main repo under the hashicorp org and just have that one reviewed.
If we want to move forward with this one, we would still need to create a draft PR under the hashicorp repo so CI will trigger and run successfully. That PRs run would have the same commits, so it will make the GH Checks pass on this PR. i am working with a community member and when I created a PR to verify the tests, it had the advantageous effect of being used as the checks in the original community PR. The basics of what I did were:

  • git checkout main
  • git pull
  • git checkout -b jm/endpoint-slices
  • git remote add <fork owner> https://github.com/<fork owner>/consul-k8s.git (in my case git remote add jukie https://github.com/jukie/consul-k8s.git )
  • git pull <fork owner> <fork branch> (in my case git pull jukie feat/zone-topology-meta)
    at this point, I have an additional remote configured so I need to make sure to use it when pulling and pushing.

@tgross
Copy link
Member Author

tgross commented Mar 18, 2024

The easiest thing to do since you are in the HashiCorp org is to create a PR from the main repo under the hashicorp org and just have that one reviewed.

Oh yeah totally what I would have done originally. But it's unfortunately not enough to be a member of the HashiCorp org, you also have to be added as a committer to the repo:

$ git push -u upstream
ERROR: Permission to hashicorp/consul-k8s.git denied to tgross.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@tgross
Copy link
Member Author

tgross commented Mar 20, 2024

Closing in lieu of #3794, which was pushed directly now that we've got GitHub permissions sorted.

@tgross tgross closed this Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x This release branch is no longer active. backport/1.3.x This release branch is no longer active. backport/1.4.x pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants