HOSTEDCP-2193: allow reuse of SP for e2e #5194
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Patryk-Stefanski: This pull request references HOSTEDCP-2193 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. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Patryk-Stefanski 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 |
openshift-ci
bot
added
area/testing
| return nil, fmt.Errorf("failed to unmarshal --managed-identities-file: %w", err) | ||
| } | ||
|
|
||
| components := map[string]string{ |
There was a problem hiding this comment.
Where are the certificate names read in?
There was a problem hiding this comment.
They're read in from the file along with the clientIDs
There was a problem hiding this comment.
I see. You were just reading out the client IDs for the role assignments.
cmd/infra/azure/create.go
Outdated
| @@ -219,6 +219,39 @@ func (o *CreateInfraOptions) Run(ctx context.Context, l logr.Logger) (*CreateInf | |||
| l.Info("Successfully created vnet", "ID", result.VNetID) | |||
| } | |||
|
|
|||
| if o.ManagedIdentitiesFile != "" { | |||
| // need to decode again in case it's run as part of an e2e | |||
There was a problem hiding this comment.
Where is this decoded the first time?
There was a problem hiding this comment.
This was before I realised we don't need the one i deleted in the complete function because this one here covers it.
There was a problem hiding this comment.
Ah ok, nit: remove the comment
cmd/infra/azure/create.go
Outdated
|
|
||
| scopes := []string{managedRG} | ||
|
|
||
| cmdStr := fmt.Sprintf("az role assignment create --assignee-object-id %s --role \"Contributor\" --scope %s --assignee-principal-type \"ServicePrincipal\" ", asigneeID, managedRG) |
There was a problem hiding this comment.
Yep duplicate line from 922
| return objectID, nil | ||
| } | ||
|
|
||
| func execAzCommand(cmdStr string) ([]byte, error) { |
There was a problem hiding this comment.
This is almost the same as createServicePrincipalWithCertificate. I would just rename the original function and reuse the same one instead of duplicating it.
There was a problem hiding this comment.
Refactoring that function to do anything else that it currently does would mean the e2es would fail until we merged in the corresponding changes in opemshift release. We can't refactor logic we have to add new logic then update the release repo logic and only then we can cleanup the old logic of creating SPs
There was a problem hiding this comment.
Is this because it can't use the CLI from the PR?
| return "", fmt.Errorf("failed to find object ID for service principal, %s : %w", appID, err) | ||
|
|
||
| } | ||
| objectID := strings.ReplaceAll(string(output), "\n", "") |
There was a problem hiding this comment.
This can be removed if you use createServicePrincipalWithCertificate since it already did this.
There was a problem hiding this comment.
That function returns the id based on the output from creating a sp and we can't refactor these commands since they're actively being used by our e2e tests
Patryk-Stefanski
force-pushed
the
HOSTEDCP-2193
branch
from
1a879dd
to
6a69742
Compare
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist