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

Ready For Review: in response to Automate Onboarding to Cluster via CLI #24 #44

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

Gregory-Pereira
Copy link
Member

#24

@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 23, 2021
README.md Outdated Show resolved Hide resolved
api/CreateGroup.go Outdated Show resolved Hide resolved
api/CreateProject.go Outdated Show resolved Hide resolved
api/Onboard.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Gregory-Pereira Gregory-Pereira force-pushed the automated-releases branch 2 times, most recently from 8e80360 to 1ea33c3 Compare August 31, 2021 16:07
cmd/onboard.go Outdated Show resolved Hide resolved
models/OnboardClusterConfig.go Outdated Show resolved Hide resolved
api/Onboard.go Show resolved Hide resolved
@HumairAK
Copy link
Member

HumairAK commented Sep 10, 2021

@Gregory-Pereira first thing you should always do is make sure the make command works, in this case it doesn't:

$ make
go build -o opfcli-linux-amd64 -ldflags "-X 'github.com/operate-first/opfcli/version.BuildVersion=unknown' -X 'github.com/operate-first/opfcli/version.BuildHash=1ea33c3d36' -X 'github.com/operate-first/opfcli/version.BuildDate=2021-09-10T14:18:39'"
# github.com/operate-first/opfcli/models
models/OnboardClusterConfig.go:4:2: imported and not used: "fmt"
make: *** [Makefile:16: opfcli-linux-amd64] Error 2

There are a couple of compilation errors that you will find when you try to run this and fix the issues as they come.

(these are also illustrated in the review check failures)

cmd/onboard.go Outdated Show resolved Hide resolved
api/Onboard.go Outdated Show resolved Hide resolved
api/Onboard.go Outdated Show resolved Hide resolved
@HumairAK
Copy link
Member

@Gregory-Pereira the make still fails. See the checks above, and also try running it locally.

@HumairAK
Copy link
Member

@Gregory-Pereira ci check still failing

@Gregory-Pereira
Copy link
Member Author

Yeah, I need to build out and fix the test suite which I will complete this week. I will message you when it's done.

@Gregory-Pereira
Copy link
Member Author

@HumairAK all tests pass, take a look when you can

@Gregory-Pereira Gregory-Pereira force-pushed the automated-releases branch 2 times, most recently from 03febdf to 142ae73 Compare March 11, 2022 23:58
@Gregory-Pereira
Copy link
Member Author

Commits have been restructured, pre-commit passes for each individual commit (including test suites, make and the actual linting). Only unresolved suggestion (logging is awaiting review. @larsks Could you take a look at the commit structure / messages and see if that is clear enough? If not I can try to refactor again. We are aiming to get this merged by end of day Monday.

@harshad16
Copy link
Member

Tested on my local , by build the binary from this pr.
it seems to be working well for the onboard
well done 🎖️
/lgtm

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2022
Copy link

@4n4nd 4n4nd left a comment

Choose a reason for hiding this comment

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

/lgtm

README.md 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
README.md 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
@sesheta sesheta removed the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2022
@HumairAK
Copy link
Member

/lgtm
/approve

Amazing work @Gregory-Pereira !

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022
@sesheta
Copy link
Member

sesheta commented Mar 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 4n4nd, HumairAK

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
@sesheta sesheta merged commit 460b3e2 into operate-first:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants