Replace wif models and client with sdk #643

JakobGray · 2024-07-19T15:45:04Z

This PR will

Update to the latest version of the ocm-sdk
Remove old WIF models and custom client code and replace them with the sdk
Update all reference to the old models in cmd/ocm/gcp with the official SDK models and client

It is not intended to introduce changes in functionality; however some changes were necessary to handle custom roles.

JakobGray · 2024-07-26T16:24:26Z

@ckandag @renan-campos

cmd/ocm/gcp/create-wif-config.go

renan-campos

Please use audience and impersonator email from the sdk.

JakobGray · 2024-07-30T16:05:04Z

I found issues with the wif templates that need to be addressed before this is merged

ckandag · 2024-07-30T17:39:43Z

cmd/ocm/gcp/delete-wif-config.go

 	if err != nil {
-		log.Fatalf("failed to create backend client: %v", err)
+		log.Fatal(err)


what does the end user see when this happens ? error stack ?

I thought we previously discussed bubbling up the errors to the caller and allow the top level "Run" function decide how to handle/display the error.

I dont see these kind of Fatal log msgs anywhere in the else in the ocm or rosa cli code

An example error:

➜ go run ./cmd/ocm gcp delete wif-config 2crdacmjbo3jmdgfn4dq791a7nig04qg 2024/07/30 14:25:07 Not logged in, credentials aren't set, run the 'login' command exit status 1

This is the top level Run function.

ckandag · 2024-07-30T18:04:45Z

@JakobGray : Across the board please fix error handling to be similar to be rest of ocm cli.
Please remove all the Fatal stmt and bubble errors up to caller. This allows for the error handling/ display to be addressed better and consistently across the CLI

JakobGray · 2024-07-30T18:35:17Z

@JakobGray : Across the board please fix error handling to be similar to be rest of ocm cli. Please remove all the Fatal stmt and bubble errors up to caller. This allows for the error handling/ display to be addressed better and consistently across the CLI

Because this PR is not intended to introduce changes in functionality I don't want to extend it's scope to address all error handling. I believe we have a ticket already to address error handling more gracefully so I think that kind of change is better addressed there.

ckandag · 2024-07-30T20:55:38Z

@JakobGray : Across the board please fix error handling to be similar to be rest of ocm cli. Please remove all the Fatal stmt and bubble errors up to caller. This allows for the error handling/ display to be addressed better and consistently across the CLI

Because this PR is not intended to introduce changes in functionality I don't want to extend it's scope to address all error handling. I believe we have a ticket already to address error handling more gracefully so I think that kind of change is better addressed there.

@JakobGray I raised this in the original MR as well
#619 (comment)

Can we please sort this out now rather than later you are anyway touching all the same files
, may be you can do it in a separate commit .
I am not sure why we decided to let this go during the initial checkin, now seems like the time to get this right.

JakobGray · 2024-07-30T22:26:29Z

The original MR addressed it by removing any panics and surfacing all errors to the top level function. This latest commit takes it a step further by removing all log.Fatal's from the top level function as well.

renan-campos · 2024-07-31T14:33:30Z

ocm-gcp-create-wif-config.log

-416843e OSD-24332 Adding CNI Type to the printed output. -ca71863 Introduce gcp WIF sub-commands to manage wif-configs (#619) -5f9697b multi arch release images (#631) -951d7cd Red Hat Konflux update ocm-cli (#633) -2604647 Limit Konflux Pipeline Runs (#634) -9645301 Update Konflux references (#635) -c797dfb Update Konflux references to 0dc3087 -28b521d support hermetic build (#636) -3117d6b Update Konflux references to 9eee3cf -d228140 Update Konflux references to 71270c3 -0ff233b update konflux pipeline (#641) -ae2093b Update Konflux references -2ae4aa0 Update Konflux references -bdd172b Update Konflux references to f93024e -d750acc Red Hat Konflux update ocm-cli Signed-off-by: red-hat-konflux <[email protected]> -0bbcf6e Update Konflux references -21ff6b8 Replace wif models and client with sdk (#643) -c3d52e2 Update Konflux build (#651) -8073ef8 release_version (#652) -e9a014d Update Konflux references -78317e9 Add 'wif-config' flag as cluster create option -49f4e41 Set project number on wif config creation -e441c1b Update Konflux references -ca8d9db Support listing and parameters in 'gcp get wif-config' (#656)

JakobGray added 3 commits July 19, 2024 11:39

Replace wif models and client with sdk

4df3bc8

Update wifconfig table column config

74acafb

Update ocm-sdk-go version

b15467d

JakobGray marked this pull request as ready for review July 29, 2024 14:24

renan-campos reviewed Jul 29, 2024

View reviewed changes

cmd/ocm/gcp/create-wif-config.go Outdated Show resolved Hide resolved

renan-campos reviewed Jul 29, 2024

View reviewed changes

Remove hardcoded audience and impesonator email

7049ed3

renan-campos approved these changes Jul 29, 2024

View reviewed changes

JakobGray requested a review from renan-campos July 30, 2024 16:04

JakobGray marked this pull request as draft July 30, 2024 16:05

Make adjustments for new sdk version and custom roles

98bdffe

ckandag reviewed Jul 30, 2024

View reviewed changes

Return errors in top level functions

1f2641a

JakobGray marked this pull request as ready for review July 31, 2024 12:37

JakobGray requested a review from ckandag July 31, 2024 14:20

renan-campos merged commit 21ff6b8 into openshift-online:main Jul 31, 2024
4 of 5 checks passed

JakobGray deleted the OCM-9646 branch July 31, 2024 14:36

ckandag mentioned this pull request Aug 8, 2024

Release v0.1.75 #658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace wif models and client with sdk #643

Replace wif models and client with sdk #643

JakobGray commented Jul 19, 2024 •

edited

Loading

JakobGray commented Jul 26, 2024

renan-campos left a comment

JakobGray commented Jul 30, 2024

ckandag Jul 30, 2024 •

edited

Loading

ckandag Jul 30, 2024 •

edited

Loading

JakobGray Jul 30, 2024

ckandag commented Jul 30, 2024 •

edited

Loading

JakobGray commented Jul 30, 2024

ckandag commented Jul 30, 2024 •

edited

Loading

JakobGray commented Jul 30, 2024

renan-campos commented Jul 31, 2024

Replace wif models and client with sdk #643

Replace wif models and client with sdk #643

Conversation

JakobGray commented Jul 19, 2024 • edited Loading

JakobGray commented Jul 26, 2024

renan-campos left a comment

Choose a reason for hiding this comment

JakobGray commented Jul 30, 2024

ckandag Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

ckandag Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

JakobGray Jul 30, 2024

Choose a reason for hiding this comment

ckandag commented Jul 30, 2024 • edited Loading

JakobGray commented Jul 30, 2024

ckandag commented Jul 30, 2024 • edited Loading

JakobGray commented Jul 30, 2024

renan-campos commented Jul 31, 2024

JakobGray commented Jul 19, 2024 •

edited

Loading

ckandag Jul 30, 2024 •

edited

Loading

ckandag Jul 30, 2024 •

edited

Loading

ckandag commented Jul 30, 2024 •

edited

Loading

ckandag commented Jul 30, 2024 •

edited

Loading