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.

Sign up for GitHub

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

Jump to bottom

test connection add #12036

Merged
merged 1 commit into from
Nov 8, 2021
Merged

test connection add #12036

merged 1 commit into from
Nov 8, 2021

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Oct 19, 2021 •
edited
Loading

  • Fix connection JSON encoding
  • Add custom ginkgo matchers for connection testing
  • Cleanup code

Fixes #11984

Signed-off-by: Jhon Honce [email protected]

@jwhonce jwhonce added the kind/bug Categorizes issue or PR as related to a bug. label Oct 19, 2021
@jwhonce jwhonce requested review from cdoern and ashley-cui October 19, 2021 15:40
@jwhonce jwhonce self-assigned this Oct 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021
edsantiago
edsantiago reviewed Oct 19, 2021
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I think you accidentally helpers.bash?

cdoern
cdoern reviewed Oct 19, 2021
View reviewed changes
test/utils/matchers.go Show resolved Hide resolved
test/utils/utils.go Show resolved Hide resolved
test/utils/matchers.go
negatedFailureMessage string
}

func VerifyService(name, uri, identity interface{}) OmegaMatcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this validation of a given service, but should we add, in CI a generic way to actually test the given connection? Not sure if this already exists but could be helpful for some basic use cases

Copy link
Member Author

Choose a reason for hiding this comment

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

@cdoern Define "test" that you desire. Round trip ping? Just a connection?

Copy link
Contributor

Choose a reason for hiding this comment

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

just a simple connection would be useful to validate what the user has given is valid syntax and a valid url

test/e2e/system_connection_test.go Outdated Show resolved Hide resolved
@TomSweeneyRedHat
Copy link
Member

@jwhonce all kinds of unhappy tests

@edsantiago
Copy link
Member

Would you mind removing helpers.bash from this?

@jwhonce jwhonce force-pushed the issues/11984 branch 5 times, most recently from ad0ca37 to f28d195 Compare October 27, 2021 23:27
@jwhonce
Copy link
Member Author

jwhonce commented Oct 27, 2021

Would you mind removing helpers.bash from this?

Already done.

@jwhonce jwhonce force-pushed the issues/11984 branch 2 times, most recently from 3d1e956 to 9d65df5 Compare October 28, 2021 03:02
@jwhonce jwhonce marked this pull request as draft October 28, 2021 03:03
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2021
@jwhonce jwhonce force-pushed the issues/11984 branch 3 times, most recently from 5fc3034 to 87ab0f9 Compare October 28, 2021 16:21
@jwhonce
Copy link
Member Author

jwhonce commented Oct 28, 2021

@cevich I see in lib.sh where the ssh key generation should happen but when the test code runs ~/.ssh has only the authorized_keys file. For tests to succeed they need the key pair installed in ~/.ssh

@cevich
Copy link
Member

cevich commented Oct 29, 2021

@jwhonce this is an easy area for confusion. That function runs as root, so we generate a key for root and authorize it for the user. So all you need is a second go at that, but as the user...

@cevich cevich mentioned this pull request Oct 29, 2021
Merged
@cevich
Copy link
Member

cevich commented Oct 29, 2021

...okay, that PR should help you out (assuming its CI passes)

@jwhonce jwhonce force-pushed the issues/11984 branch 13 times, most recently from b82e66d to 5e06ffa Compare November 5, 2021 17:33
@jwhonce jwhonce marked this pull request as ready for review November 5, 2021 21:05
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2021
@jwhonce
Copy link
Member Author

jwhonce commented Nov 5, 2021

@mheon @baude @Luap99 @flouthoc @cevich PTAL

@jwhonce
Copy link
Member Author

jwhonce commented Nov 8, 2021

@containers/podman-maintainers PTAL

@jwhonce
test connection add
e907f09 
* Fix connection JSON encoding
* Add custom ginkgo matchers for connection testing
* Cleanup code

Fixes containers#11984

Signed-off-by: Jhon Honce <[email protected]>
@flouthoc
Copy link
Collaborator

flouthoc commented Nov 8, 2021

@jwhonce LGTM. Just the changes in tests were too out-of-context for me so I'll wait for CI to verify that.

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit d0a4475 into containers:main Nov 8, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@cdoern cdoern cdoern left review comments

@edsantiago edsantiago edsantiago left review comments

@ashley-cui ashley-cui Awaiting requested review from ashley-cui

Assignees

@jwhonce jwhonce

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error: failed to parse 'podman info' results: json: cannot unmarshal string into Go value of type define.Info
9 participants
@jwhonce @TomSweeneyRedHat @edsantiago @cevich @flouthoc @rhatdan @cdoern @Luap99 @openshift-merge-robot