Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The CLI Client #198

Merged
merged 59 commits into from
Jul 17, 2024
Merged

The CLI Client #198

merged 59 commits into from
Jul 17, 2024

Conversation

zhijie-yang
Copy link
Contributor

@zhijie-yang zhijie-yang commented Jun 11, 2024

Ping the @canonical/rocks team.


Description

This pull request implements a cli client for OCI Factory, such that it is no longer required to create pull requests to trigger image builds and releases for the images.

For details see specification RK040.

zhijie-yang and others added 19 commits June 4, 2024 18:35
 * add workflow dispatch client
 * add build metadata inferer
 * Moved main logic to cli_main
 * Polling happens in cli_main. client.wf_poller just leave the
   API to query workflow run status
 * TODO optimize out redundent passing `accessToken`s as func
   args
 * add spiners to indicate waiting for workflow run to show up
@zhijie-yang zhijie-yang force-pushed the ROCKS-1216-cli-client branch 2 times, most recently from 3db7a78 to c8e3028 Compare June 13, 2024 13:28
@zhijie-yang zhijie-yang marked this pull request as ready for review June 14, 2024 08:06
Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty for the changes. just a bunch of nits, and over @rebornplusplus approves it we should be good to merge it

.github/workflows/CLI-Client.yaml Outdated Show resolved Hide resolved
.github/workflows/CLI-Client.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cli-client/snap/snapcraft.yaml Show resolved Hide resolved
src/cli-client/snap/snapcraft.yaml Outdated Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass. An overall go-specific review. Please check if the functionality works or not.

Error handling needs to be a bit better in my opinion. In lots of places, we are trying to handle the errors from within the function by printing or panicking, which although would most likely work, is not a nice way to do it and on the rarest of occasions, might just break. Let's change those up to return said errors from the function.

func foo(args Type) (RetType, error) {
	...
	// return errors whenever something does not work
	...
}

...

val, err := foo(bar)
if err != nil {
	// Handle the error here.
	logger.Panicf("foo does not work: %v", err)
}

If, indeed, error-handling within the function is a must, we can do something like this too:

func foo(args Type) (ret RetType, err error) {
	defer func() {
		if err != nil {
			// Handle the error here. Example:
			logger.Panicf(err)
		}
	}()
	...
}

src/cli-client/go.mod Show resolved Hide resolved
src/cli-client/go.mod Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_main.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_main.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_main.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_main.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_upload.go Show resolved Hide resolved
src/cli-client/internals/cli/cli_upload.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_upload.go Outdated Show resolved Hide resolved
src/cli-client/internals/cli/cli_upload.go Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the go stuffs -- looks OK to me for a first start. Nice work!

PS. please check whether it works properly or not :) I don't have much context about oci-factory.

src/cli-client/internals/cli/cli_upload.go Show resolved Hide resolved
Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can u pls ensure all comments are resolved?

@zhijie-yang
Copy link
Contributor Author

All resolved.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks

@cjdcordeiro cjdcordeiro merged commit 5ed0399 into main Jul 17, 2024
6 checks passed
@cjdcordeiro cjdcordeiro deleted the ROCKS-1216-cli-client branch July 17, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants