CCXDEV-14850: align insights DataGather with config #2248
Conversation
This commit introduces the v1alpha2 version of the DataGather CRD for Insights. Signed-off-by: Ondrej Pokorny <[email protected]>
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-14850 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 task to target the "4.19.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. |
|
Hello @opokornyy! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: opokornyy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
from
cce4ef9 to
af4174b
Compare
This commit aligns the configuration options of the DataGather CRD with those of InsightsDataGather, making it easier to use both CRDs. Signed-off-by: Ondrej Pokorny <[email protected]>
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
from
af4174b to
4bada1b
Compare
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
2 times, most recently
from
4725f08 to
866a461
Compare
|
Just leaving a general note that some of the changes I've requested are a deviation from the v1alpha1 API that I think would be inherently breaking (i.e removing fields, etc.). I'm still learning what this means for alpha level APIs that are only in TPNU clusters, but I suspect we would likely want to go back and bring the v1alpha1 APIs into alignment (or drop them?). @JoelSpeed what is the correct approach to follow here? |
|
Since we aren't going to allow clusters to upgrade between v1alpha1 and v1alpha2, we don't need to make changes in the alpha 1 or care about conversion webhooks, so we can make breaking changes here |
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
3 times, most recently
from
811a9a2 to
d548836
Compare
There was a problem hiding this comment.
A couple more comments plus my still outstanding comment on the TotalRisk field.
Other than that, I think this looks good.
@JoelSpeed will need to make a pass on it prior to it being able to merge though
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
from
d548836 to
0fa9cfb
Compare
There was a problem hiding this comment.
A couple more comments. Other than these, I think it looks good.
tagging in @JoelSpeed for a review.
insights/v1alpha2/types_insights.go
Outdated
| // When it has a status of False and a reason Failed, the upload failed. The accompanying message will include the specific error encountered. | ||
| // | ||
| // The DataRecorded condition is used to represent whether or not the archive was successfully recorded. | ||
| // When it has a status of True and a reason of AsExpected, the archive was recorded successfully. |
There was a problem hiding this comment.
Succeeded > AsExpected ? WDYT?
There was a problem hiding this comment.
I have updated the values, so you can check it
insights/v1alpha2/types_insights.go
Outdated
| // The RemoteConfigurationAvailable condition is used to represent whether the remote configuration is available. | ||
| // When it has a status of Unknown and a reason of Unknown or RemoteConfigNotRequestedYet, the state of the remote configuration is unknown—typically at startup. | ||
| // When it has a status of True and a reason of AsExpected, the configuration is available. | ||
| // When it has a status of False and a reason of AsExpected, the configuration is not available. |
There was a problem hiding this comment.
Is AsExpected the right reason here? Reads a little funky to me if both available and unavailable are an expected state
There was a problem hiding this comment.
Changed to Succeeded and Invalid
insights/v1alpha2/types_insights.go
Outdated
| // The RemoteConfigurationValid condition is used to represent whether the remote configuration is valid. | ||
| // When it has a status of Unknown and a reason of Unknown or NoValidationYet, the validity of the remote configuration is unknown—typically at startup. | ||
| // When it has a status of True and a reason of AsExpected, the configuration is valid. | ||
| // When it has a status of False and a reason of AsExpected, the configuration is invalid. |
There was a problem hiding this comment.
Ditto re: AsExpected when status is False
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
from
0fa9cfb to
882784e
Compare
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
2 times, most recently
from
5a4f9e6 to
bb217af
Compare
Signed-off-by: Ondrej Pokorny <[email protected]>
opokornyy
force-pushed
the
CCXDEV-15040-insights-v1alpha2
branch
from
bb217af to
522fbdc
Compare
|
@opokornyy: 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. |
This PR should align the configuration options in DataGather with the InsightsDataGather