[4.21] OCPBUGS-59514: redacted install config for baremetal #9972
Conversation
openshift-ci-robot
added
jira/severity-important
|
@iurygregory: This pull request references Jira Issue OCPBUGS-59514, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@iurygregory: This pull request references Jira Issue OCPBUGS-59514, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
iurygregory
force-pushed
the
OCPBUGS-59514
branch
from
2d85a68 to
2f70ace
Compare
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
/label backport-risk-assessed
Hm perhaps I'm missing something obvious, but could you simply do this:
case newConfig.BareMetal != nil:
for _, host := range newConfig.BareMetal.Hosts {
host.BMC.Password = "REDACTED"
}This way you would not need to update this code for future fields. Not sure why other platforms don't do this, so perhaps I am missing something!
openshift-ci
bot
added
the
backport-risk-assessed
|
@patrickdillon thanks for the feedback, let me push a change with your idea and test via cluster-bot! |
iurygregory
force-pushed
the
OCPBUGS-59514
branch
from
2f70ace to
749c837
Compare
|
It looks like another instance of this problem was already fixed by OCPBUGS-61353, but there is still one more that this doesn't solve. Clearly this is going to keep happening though. I don't feel like this is fixed until we have code that uses
I think the problem is that this changes the original install-config, meaning the redacted version will get written to the asset store. The InstallConfig struct is passed by value, so it is safe to overwrite the pullSecret at the top level, but it contains references to nested structs (Platform struct is embedded by value, but it contains pointers to the actual platform implementation), and only the references get copied. We need to either go through (a hopefully better version of) the same rigmarole as the other platforms or generate |
+1. I knew I was missing something and this point was exactly it. Ignore my suggestion! |
|
& apologies for misleading you @iurygregory |
iurygregory
force-pushed
the
OCPBUGS-59514
branch
3 times, most recently
from
1cb7810 to
5e4737a
Compare
Looks like you predicted #10025, Zane. This PR should make the whole copying much easier and individual overrides can be done just like Patrick's previous suggestion. |
Looks like a good scenarios to use Claude 😁 Current approach looks OK to me though resolve the issue today unless you want to wait for #10025, @iurygregory |
Hey @tthvo , I'll be waiting for #10025 to land so I can update things! |
Thanks @patrickdillon I will wait for #10025 to merge so I can update this one. |
|
@iurygregory #10025 is merged. You can use deepcopy now :D |
|
@tthvo thanks! going to update here |
iurygregory
force-pushed
the
OCPBUGS-59514
branch
from
5e4737a to
8839bdf
Compare
iurygregory
force-pushed
the
OCPBUGS-59514
branch
from
8839bdf to
d19dfa9
Compare
|
/jira refresh |
|
@iurygregory: This pull request references Jira Issue OCPBUGS-59514, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
Assisted-By: Claude Code Sonnet 4 Signed-off-by: Iury Gregory Melo Ferreira <imelofer@redhat.com>
iurygregory
force-pushed
the
OCPBUGS-59514
branch
from
d19dfa9 to
4b09c16
Compare
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Looks good! The unit tests also verifies that it works and does not break other platform.
/cc @jcpowermac for vsphere 🙏
|
/retest Hmm, test failures complain about missing |
|
@jcpowermac can you take a look at the vsphere part? Tks! |
|
/lgtm |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test ? |
|
@patrickdillon: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn 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 kubernetes-sigs/prow repository. |
|
/test e2e-metal-ipi-ovn e2e-vsphere-ovn e2e-nutanix-ovn |
|
@iurygregory: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
7 participants
No description provided.