Skip to content

Conversation

@r4f4
Copy link
Contributor

@r4f4 r4f4 commented Dec 15, 2022

No description provided.

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 16, 2022

Dropped 83d0a6a since it merged as part of #6601

Copy link
Contributor

@barbacbd barbacbd left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link
Contributor

Choose a reason for hiding this comment

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

this would be one I'm interested in learning more about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll follow up with the agent-installer team on this one. gosec always expects 0600 or less.

Copy link
Contributor

@bfournie bfournie Dec 16, 2022

Choose a reason for hiding this comment

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

Follow up on slack discussion - I tested it using 0600 and its fine, I will create a PR to change this

Comment on lines +180 to +186
switch {
case tc.expectedError != "":
assert.ErrorContains(t, err, tc.expectedError)
} else if len(tc.expectedConfig) == 0 {
case len(tc.expectedConfig) == 0:
assert.NoError(t, err)
assert.Equal(t, tc.expectedConfig, asset.Config)
} else {
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps overly prescriptive, but I kind of like it

mac-address: 52:54:01:aa:aa:a1`

// config with route but no interface is invalid
// config with route but no interface is invalid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... I didn't think godot would dot this kind of comment... I was curious what they meant by "top-level"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the default is declarations [1]. But what that means is based on the Golang's AST [2]. I'll play around with the settings if we start getting too many of these.

[1] https://golangci-lint.run/usage/linters/#godot
[2] tetafro/godot#22 (comment)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Details 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 Dec 16, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 4442bbd and 2 for PR HEAD 607d606 in total

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 16, 2022

/skip

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 16, 2022

/override ci/prow/e2e-azure-ovn ci/prow/e2e-vsphere-ovn
Previous runs installed but timed out either during deprovision or during tests.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2022

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn, ci/prow/e2e-vsphere-ovn

Details

In response to this:

/override ci/prow/e2e-azure-ovn ci/prow/e2e-vsphere-ovn
Previous runs installed but timed out either during deprovision or during tests.

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.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 9ee3437 and 1 for PR HEAD 607d606 in total

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 17, 2022

/override ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 17, 2022

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn, ci/prow/e2e-gcp-ovn, ci/prow/e2e-vsphere-ovn

Details

In response to this:

/override ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn

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.

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 17, 2022

/override ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn ci/prow/e2e-aws-ovn

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 17, 2022

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-aws-ovn, ci/prow/e2e-azure-ovn, ci/prow/e2e-gcp-ovn, ci/prow/e2e-vsphere-ovn

Details

In response to this:

/override ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn ci/prow/e2e-aws-ovn

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.

@openshift-merge-robot openshift-merge-robot merged commit 08d4c38 into openshift:master Dec 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 17, 2022

@r4f4: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-e2e-aws-ovn 607d606 link false /test okd-scos-e2e-aws-ovn
ci/prow/okd-e2e-aws-ovn-upgrade 607d606 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-openstack-sdn-parallel 607d606 link false /test e2e-openstack-sdn-parallel
ci/prow/e2e-openstack-kuryr 607d606 link false /test e2e-openstack-kuryr
ci/prow/e2e-libvirt 607d606 link false /test e2e-libvirt
ci/prow/e2e-openstack 607d606 link false /test e2e-openstack
ci/prow/e2e-ovirt-sdn 607d606 link false /test e2e-ovirt-sdn
ci/prow/e2e-aws-ovn-disruptive 607d606 link false /test e2e-aws-ovn-disruptive
ci/prow/e2e-ibmcloud-ovn 607d606 link false /test e2e-ibmcloud-ovn
ci/prow/e2e-openstack-proxy 607d606 link false /test e2e-openstack-proxy
ci/prow/e2e-alibaba 607d606 link false /test e2e-alibaba

Full PR test history. Your PR dashboard.

Details

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.

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