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

Update README with ssh key information #24

Merged

Conversation

ravisantoshgudimetla
Copy link
Contributor

cc @aravindhp @akhil-rane @openshift/openshift-team-windows-containers

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2020
@ravisantoshgudimetla
Copy link
Contributor Author

/approve cancel

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2020
@akhil-rane
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2020
README.md Outdated
@@ -19,8 +19,11 @@ To run the e2e tests for WMCO locally against an OpenShift cluster set up on AWS
export KUBECONFIG=<path to kubeconfig>
export AWS_SHARED_CREDENTIALS_FILE=<path to aws credentials file>
export CLUSTER_ADDR=<cluster_name, eg: ravig211.devcluster.openshift.com>
export KUBE_SSH_KEY_PATH=<path to ssh key>
export KUBE_SSH_KEY_PATH=<path to ssh key>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extra space. Will remove it.

Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @ravisantoshgudimetla. Please address my comments.

README.md Outdated
Comment on lines 24 to 25
Make sure that the key you're using in KUBE_SSH_KEY_PATH is
private key part of the keypair mentioned at [sshKeyPair](https://github.com/openshift/windows-machine-config-operator/blob/42593c5d2eb798b572c58b5debafc4c392d1f967/test/e2e/wmco_test.go#L59) within the instance CR
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that you need to change KUBE_SSH_KEY_PATH to point to libra which we cannot do in development. This should rather say, Update the keypair mentioned at sshKeyPair to match what is being used in KUBE_SSH_KEY_PATH. Also call out that libra is being used for CI purposes.

BTW, did you try hard coding to openshift-dev? I think the test platform team has uploaded that key to all AWS regions in the CI account when they decided to do region sharding for CI. Please try this out in a separate PR and move to using that if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, did you try hard coding to openshift-dev? I think the test platform team has uploaded that key to all AWS regions in the CI account when they decided to do region sharding for CI. Please try this out in a separate PR and move to using that if it works.

I tried it multiple times in the CI for #15 and it failed.

This implies that you need to change KUBE_SSH_KEY_PATH to point to libra which we cannot do in development. This should rather say, Update the keypair mentioned at sshKeyPair to match what is being used in KUBE_SSH_KEY_PATH. Also call out that libra is being used for CI purposes.

Will do that.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020
Copy link
Contributor

@aravindhp aravindhp left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020
@ravisantoshgudimetla
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020
@akhil-rane
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@aravindhp
Copy link
Contributor

/hold

Until #23 merges

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020
@ravisantoshgudimetla
Copy link
Contributor Author

/retest

@ravisantoshgudimetla
Copy link
Contributor Author

/hold cancel

as #23 merged

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit f8342b3 into openshift:master Mar 24, 2020
@openshift-ci-robot
Copy link

@ravisantoshgudimetla: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-operator 2e8fdc8 link /test e2e-operator

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wgahnagl pushed a commit to wgahnagl/windows-machine-config-operator that referenced this pull request Mar 6, 2023
wgahnagl pushed a commit to wgahnagl/windows-machine-config-operator that referenced this pull request Mar 6, 2023
As of now, go.sum on master branch is broken.
The PR openshift#24 broke it. This commit should fix
the broken go.sum. We need to run `go mod
verify` in the wni directory to generate the
go.sum file again, building from wmco location
using make build-tools won't generate the go.sum
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants