OCPEDGE-1168: change DevicePath field to its own type #690
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@qJkee: This pull request references OCPEDGE-1168 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qJkee 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
- Coverage 70.66% 70.53% -0.13%
==========================================
Files 47 48 +1
Lines 3245 3262 +17
==========================================
+ Hits 2293 2301 +8
- Misses 785 791 +6
- Partials 167 170 +3
|
qJkee
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| @@ -31,7 +31,7 @@ func parseMetrics(metricsResult string) ([]RawMetric, error) { | |||
| return nil, fmt.Errorf("failed to unmarshal metrics: %w", err) | |||
| } | |||
|
|
|||
| metrics := []RawMetric{} | |||
| metrics := make([]RawMetric, 0) | |||
There was a problem hiding this comment.
| metrics := make([]RawMetric, 0) | |
| var metrics []RawMetric |
There was a problem hiding this comment.
I think this is the last nit pending
match golang version in debug dockerfile with main dockerfile
openshift-ci
bot
added
size/XL
|
/hold for review |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Mostly nits, approach is LGTM for me.
Looking at the code for the resolver, it feels a little like we do not need the concurrent sync.Map access since we run in the reconcile and the reconcile doesnt have a concurrent call for device resolution AFAIK. (please correct me if im wrong)
Im also not sure if we should keep the DevicePath type because now its literally a string wrapper, although I must admit that the Unresolved() method makes it clear what is to be expected from that value. maybe a godoc would resolve that?
|
/test e2e-aws |
qJkee
force-pushed
the
OCPEDGE-1168
branch
2 times, most recently
from
438dc28 to
8ef7937
Compare
Add new object for symlink resolution with caching to reduce symlink resolution amount within reconcile loop Also, introduce a new type called DevicePath to contain user provided device path. This is done in order to create a standard way of working with raw(unresolved) device path and avoid errors when it's not resolved while it's expected to be.
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@qJkee: 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. |
Introduce a new type called DevicePath to contain user provided device path. This is done in order to create a standard way of working with resolved if symlink or raw(unresolved) device path and avoid errors when it's not resolved while it's expected to be.