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.

Sign up for GitHub

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

Jump to bottom

[ocu-472] basic smithyctl boilerplate #705

Closed
wants to merge 6 commits into from
Closed

[ocu-472] basic smithyctl boilerplate #705

wants to merge 6 commits into from

Conversation

andream16
Copy link
Contributor

@andream16 andream16 commented Jan 15, 2025 •
edited
Loading

  • Adding boilerplate for smithyctl commands. Will open another PR shortly implementing packaging.
  • Fixing a small bug on V1 Types parsing. Also moving exported code in smithyctl/pkg
  • Making sure that we can test every single go test file on CI - some were being skipped and now it's fixed. Opened https://linear.app/smithy/issue/OCU-482/make-sure-git-cloner-integration-test-works-on-ci to tackle an issue that I found on CI with a test. I spent some time on it but it would be better for me to focus on what matters more right now. I have an action to work on this when I have more time.

@andream16 andream16 force-pushed the andream16/OCU-472/basic-smithyctl-boilerplate branch 5 times, most recently from 55da1f9 to 04970de Compare January 15, 2025 15:54
@andream16 andream16 marked this pull request as ready for review January 15, 2025 15:56
@andream16 andream16 self-assigned this Jan 15, 2025
andream16
andream16 commented Jan 15, 2025
View reviewed changes
smithyctl/pkg/types/v1/parameter.go Outdated
@@ -57,7 +57,9 @@ func (p *Parameter) UnmarshalJSON(b []byte) error {
case ParameterTypeString, ParameterTypeConststring:
parameterValueStrPtr := &struct{ Value *string }{}
if err = json.Unmarshal(b, parameterValueStrPtr); err == nil {
p.Value = parameterValueStrPtr.Value
if parameterValueStrPtr.Value != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small fix to an issue that I discovered while re-checking all the tests.

Makefile
@@ -133,7 +134,10 @@ install-go-test-tools:

go-tests:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now covers EVERY possible go test file in the repo. Before there were some issues due to multiple go.mods being present and test runs being hidden on CI.

Now this enable for a sane framework

Copy link
Contributor

Choose a reason for hiding this comment

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

this can be simplified to find . -name go.mod -execdir go test ./... \;

you shouldn't be looking for _test.go files but for go.mod files

ptzianos
ptzianos reviewed Jan 16, 2025
View reviewed changes
smithyctl/cmd/main.go Outdated
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()

if err := Main(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

the two mains can be merged, I don't see the point in a public Main

smithyctl/internal/command/version/command.go
import "github.com/spf13/cobra"

// NewCommand returns a new version command.
func NewCommand() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can implement the version already by injecting a variable with the -X flag during build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't mind let's do it in its ticket

Makefile
@@ -133,7 +134,10 @@ install-go-test-tools:

go-tests:
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be simplified to find . -name go.mod -execdir go test ./... \;

you shouldn't be looking for _test.go files but for go.mod files

@ptzianos
Copy link
Contributor

A couple of general comments that don't fit neatly into any of the commits:

  1. please move the v1 types out of the smithyctl subtree and into the pkg subtree, they are supposed to be used in other places and they are not just for smithyctl.
  2. please move the smithyctl commands away from the internal packages and somewhere where they are available from other modules so that we can repackage them

dlicheva
dlicheva reviewed Jan 16, 2025
View reviewed changes
smithyctl/internal/command/packaging/command.go Outdated

// NewCommand returns a new package command.
func NewCommand() *cobra.Command {
// TODO: implement in the next PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a ticket associated with this todo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@andream16
Copy link
Contributor Author

A couple of general comments that don't fit neatly into any of the commits:

  1. please move the v1 types out of the smithyctl subtree and into the pkg subtree, they are supposed to be used in other places and they are not just for smithyctl.
  2. please move the smithyctl commands away from the internal packages and somewhere where they are available from other modules so that we can repackage them
  1. Done
  2. Exported commands in PKG for smithyctl

@andream16 andream16 force-pushed the andream16/OCU-472/basic-smithyctl-boilerplate branch from 04970de to d14c89b Compare January 16, 2025 16:07
@andream16 andream16 force-pushed the andream16/OCU-472/basic-smithyctl-boilerplate branch from d14c89b to 72f7a78 Compare January 16, 2025 16:08
@andream16 andream16 mentioned this pull request Jan 16, 2025
Open
@andream16
Copy link
Contributor Author

Declining in favour of #707

@andream16 andream16 closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlicheva dlicheva dlicheva left review comments

@ptzianos ptzianos ptzianos left review comments

@northdpole northdpole Awaiting requested review from northdpole

@PTruscott PTruscott Awaiting requested review from PTruscott

Assignees

@andream16 andream16

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andream16 @ptzianos @dlicheva