OSD-22892 (Part 2): Legacy Probe #241
Conversation
| "github.com/openshift/osd-network-verifier/pkg/output" | ||
| ) | ||
|
|
||
| type LegacyProbe struct{} |
There was a problem hiding this comment.
nitpick - I personally prefer to not name types starting with the package name, since it's redundant to external callers. e.g. another package calling this would look like legacy.LegacyProbe.
There was a problem hiding this comment.
I hear you; I did feel a twinge of ick while writing that exact reference in the (soon-to-be-PR'd) integration code. Do you have any suggestions of how to avoid?
There was a problem hiding this comment.
My vote would be to just go with type Probe struct{}, and then external callers would do legacy.Probe which is still clear to me.
There was a problem hiding this comment.
Might that conflict with the Probe interface though? Probably not literally due to being in different packages (although legacy is a child of probes), but mentally? Not trying to be obstinate, just want to best understand the suggestion
|
/lgtm |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafael-azevedo 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 |
openshift-ci
bot
added
the
approved
abyrne55
force-pushed
the
OSD-22892-legacy
branch
from
ad49eff to
8271c48
Compare
openshift-merge-robot
removed
the
needs-rebase
| // TODO: REPLACE JUNK VALUE "helpers.UserdataTemplate" BELOW | ||
| data := os.Expand("helpers.UserdataTemplate", variableMapper) |
There was a problem hiding this comment.
Note: this change will make the compiler happy once we delete helpers.UserdataTemplate (in a future PR; probably "Part 4"). GCP support is already broken, so this logical "non-fix" doesn't actually take anything away.
abyrne55
force-pushed
the
OSD-22892-legacy
branch
from
8271c48 to
7956aad
Compare
|
@abyrne55: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
+ Coverage 27.42% 28.81% +1.38%
==========================================
Files 11 12 +1
Lines 1072 1128 +56
==========================================
+ Hits 294 325 +31
- Misses 764 786 +22
- Partials 14 17 +3
|
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
What does this PR do? / Related Issues / Jira
This PR is composed of commits cherry-picked from my larger OSD-22892 branch for the sake of readability.
These commits introduce
LegacyProbe, which implements theProbeinterface similarly to CurlJSONProbe. The goal of this Probe is to mimic the functionality of the verifier pre-experimental-probe (i.e., v0.4.10) as closely as possible ("bug-for-bug" 😉) while still adhering to the letter and spirit of the new Probe interface. The x86 AMI IDs included in this PR are the last "golden AMI" images published to the verifier's golden AMI account: hopefully, we don't need to update these much at all.This PR intentionally does not cover integration of this new code into the verifier's critical path. That integration will be covered in a separate cherry-picked PR. As such, there isn't much to execute for this PR beyond the included unit tests, and even those will likely fail to run until #240 is merged. When reviewing this PR, please focus on code style/structure.
Checklist
[ ] I have made corresponding change to the default configuration files[ ] I have added execution results to the PR's readme