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

Added owner reference for nns to node #211

Merged
merged 5 commits into from
Oct 11, 2019
Merged

Added owner reference for nns to node #211

merged 5 commits into from
Oct 11, 2019

Conversation

pitiK3U
Copy link
Contributor

@pitiK3U pitiK3U commented Oct 9, 2019

Resolves #13

@ovirt-infra
Copy link
Member

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

  1. Type ci test please on this PR to trigger automated tests for it.
  2. Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow add status object to NodeNetworkState with nmstate error #3 option )
  3. If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

@pitiK3U pitiK3U changed the title Added onwer reference for nns to node Added owner reference for nns to node Oct 9, 2019
Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

there is a reason why we update the go modules on this PR?

@pitiK3U
Copy link
Contributor Author

pitiK3U commented Oct 9, 2019

Oh, no. Sorry I forgot to delete unused import.

phoracek
phoracek previously approved these changes Oct 9, 2019
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

Tested manually. (We can add a test removing Nodes, but we don't know how to add them back)

Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

I still see a change in the lock file.

I think you need to revert this also or the lock file will not be in sync

@SchSeba
Copy link
Contributor

SchSeba commented Oct 9, 2019

Tested manually. (We can add a test removing Nodes, but we don't know how to add them back)

@phoracek I think the best solution for this is to create another lane in the STD-CI with just one test (ginkgo focus) to remove the node.

@phoracek
Copy link
Member

phoracek commented Oct 9, 2019

@SchSeba it looks like there was an issue with the lock before and we just randomly fixed it here by running dep ensure.

@phoracek
Copy link
Member

phoracek commented Oct 9, 2019

ci test please

1 similar comment
@phoracek
Copy link
Member

phoracek commented Oct 9, 2019

ci test please

SchSeba
SchSeba previously approved these changes Oct 9, 2019
@phoracek
Copy link
Member

phoracek commented Oct 9, 2019

@SchSeba let's do the test :) thanks for not letting me to be lazy

@pitiK3U pitiK3U dismissed stale reviews from SchSeba and phoracek via fbc9fbf October 11, 2019 16:10
@phoracek
Copy link
Member

ci test please

phoracek
phoracek previously approved these changes Oct 11, 2019
phoracek
phoracek previously approved these changes Oct 11, 2019
@phoracek
Copy link
Member

ci test please

Signed-off-by: Petr Horacek <[email protected]>
@phoracek
Copy link
Member

ci test please

@phoracek phoracek merged commit ab0f4f1 into nmstate:master Oct 11, 2019
@pitiK3U pitiK3U deleted the remove-nns-on-node-remove branch October 11, 2019 18:48
creydr pushed a commit to creydr/kubernetes-nmstate that referenced this pull request Mar 1, 2022
…-pick-210-to-release-4.9

[release-4.9] Sync Dockerfile.openshift with u/s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove NodeNetworkState after Node removal
4 participants