Conversation
|
👋 tarcisiozf, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
| func ShouldSkipGetOwner(cmd *cobra.Command) bool { | ||
| switch cmd.Name() { | ||
| case "help": | ||
| case "help", "id": |
There was a problem hiding this comment.
id generation command requires owner - owner address is part of inputs to workflow ID generation.
There was a problem hiding this comment.
The whole idea behind of this feature is to be able to verify a workflow deployed by others. Since it's a external owner address this field is not required in the local config, and this is why there is an option flag to also overwrite owner address via CLI instead of changing config, so the user has both options.
anirudhwarrier
requested review from
anirudhwarrier and
timothyF95
and removed request for
Atrax1
| } | ||
|
|
||
| // optional owner flag overrides the owner from settings | ||
| cmd.Flags().String("owner", "", "Workflow owner address") |
There was a problem hiding this comment.
Is this flag needed? Not a pattern in other commands.
There was a problem hiding this comment.
This doesn't work for me without the flag. Do you have a demo?
timothyF95
left a comment
timothyF95
left a comment
There was a problem hiding this comment.
I like this feature.
Some items will need adjusting. In particular I would like to address the handling of the workflow owner.
| } | ||
|
|
||
| var defaultOutputPath = "./binary.wasm.br.b64" | ||
| const defaultOutputPath = "./binary.wasm.br.b64" |
There was a problem hiding this comment.
As this is now shared we should move it to the constants package:
./internal/constants/constants.go
| var excludedCommands = map[string]struct{}{ | ||
| "cre logout": {}, | ||
| "cre logout": {}, | ||
| "cre workflow generate-id": {}, |
There was a problem hiding this comment.
This should be removed. We want to enforce auth validation
There was a problem hiding this comment.
Discussed offline and it was decided to keep as is without validating auth
| func (h *handler) Execute() error { | ||
| h.displayWorkflowDetails() | ||
|
|
||
| if !h.validated { |
| @@ -0,0 +1,165 @@ | |||
| package build | |||
There was a problem hiding this comment.
Nice 👍 I think later we should integrate this into the flow for both sim and deploy
| @@ -0,0 +1,36 @@ | |||
| //go:build wasip1 | |||
|
|
|||
There was a problem hiding this comment.
Could we reuse the testdata from deploy cmd or hoist into an internal package to be shared?
| func ShouldSkipGetOwner(cmd *cobra.Command) bool { | ||
| switch cmd.Name() { | ||
| case "help": | ||
| case "help", "generate-id": |
There was a problem hiding this comment.
This is correct but I don't like that we have to do it.
| func New(ctx *runtime.Context) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "generate-id <workflow-folder-path>", | ||
| Short: "Display the workflow ID", |
There was a problem hiding this comment.
Could add a long desc explaining a little more on what it does
| } | ||
|
|
||
| // optional owner flag overrides the owner from settings | ||
| cmd.Flags().String("owner", "", "Workflow owner address") |
There was a problem hiding this comment.
This doesn't work for me without the flag. Do you have a demo?
| @@ -0,0 +1,132 @@ | |||
| package generate_id | |||
There was a problem hiding this comment.
We should have unit tests for input validation
./generate_id_test.go
3 participants
TICKET: DEVSVCS-2981
Refactoring and modularization:
workflowArtifactstruct with the new sharedartifact.Artifacttype throughout the deploy and register code, and updates test code accordingly.buildpackage for resolving build parameters and compiling workflows, removing duplicated code and simplifying theCompilemethod.artifact.Builderandartifact.Inputs, replacing custom logic for reading and encoding binaries and configs.New features:
workflow idsubcommand that builds the workflow and prints its computed ID, using the shared artifact and build logic. (cmd/workflow/id/id.go)General improvements:
brotli.NewWriter