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

eksctl info command #3354

Merged
merged 1 commit into from
Jun 25, 2021
Merged

Conversation

neha-viswanathan
Copy link
Contributor

@neha-viswanathan neha-viswanathan commented Feb 25, 2021

Closes #3029

Description

This MR contains an implementation of the eksctl info command

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@neha-viswanathan neha-viswanathan marked this pull request as draft February 25, 2021 06:02
@neha-viswanathan
Copy link
Contributor Author

@aclevername 👋🏽 this is a rough first attempt of the eksctl info command. Please review and let me know your suggestions.

@aclevername
Copy link
Contributor

Thanks @neha-viswanathan ! Looks good, could we do some error handling? For example if we fail to discover the kubectl client we should just present unknown or something like that 😄

@neha-viswanathan
Copy link
Contributor Author

neha-viswanathan commented Mar 4, 2021

@aclevername ready for review!

@neha-viswanathan neha-viswanathan marked this pull request as ready for review March 4, 2021 06:55
@neha-viswanathan neha-viswanathan changed the title WIP: eksctl info command eksctl info command Mar 5, 2021
@Callisto13 Callisto13 added the kind/feature New feature or request label Mar 9, 2021
@neha-viswanathan
Copy link
Contributor Author

@Callisto13 Hi! I'll need your guidance here - I don't understand why the make lint is failing? Please help!

@Callisto13
Copy link
Contributor

No problem 😃 .

So the linter is reporting this error:

errors in package: [/__w/eksctl/eksctl/cmd/eksctl/main.go:45:49: undeclared name: infoCmd]"

Because the infoCmd function you are referencing on line 45 of cmd/eksctl/main.go does not exist. (This line here: cmdutils.AddResourceCmd(flagGrouping, rootCmd, infoCmd).) You need to create that infoCmd function, which should call your new GetVersion function.

You can use the line below in cmd/eksctl/main.go which wires up the versionCmd as a guide. versionCmd is defined in cmd/eksctl/version.go, so following that pattern you can create a new cmd/eksctl/info.go which will define infoCmd and call your GetVersion function.

Hope this helps, let me know if you have any more questions or if anything is unclear 👍

@neha-viswanathan
Copy link
Contributor Author

@Callisto13 for some reason that file did not get committed earlier. 😕 Thanks for pointing it out. 😃

@neha-viswanathan neha-viswanathan marked this pull request as ready for review March 26, 2021 15:03
@Callisto13
Copy link
Contributor

for some reason that file did not get committed earlier

Gah! that's annoying 😅

pkg/info/info.go Outdated
}

// GetVersion returns versions info
func GetVersion() Version {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is possible to have some tests for this file please?

@neha-viswanathan neha-viswanathan marked this pull request as draft March 26, 2021 15:46
@neha-viswanathan neha-viswanathan changed the title eksctl info command WIP: eksctl info command May 5, 2021
@Callisto13
Copy link
Contributor

Hey @neha-viswanathan! Just checking in on the status of this, do you plan to finish or would you like it taken over? Thanks!

@neha-viswanathan
Copy link
Contributor Author

Hey @neha-viswanathan! Just checking in on the status of this, do you plan to finish or would you like it taken over? Thanks!

Hi @Callisto13 I'll try and wrap this up within the next week or so. I will keep you posted in any case.

@Callisto13
Copy link
Contributor

Awesome 🎉

@neha-viswanathan neha-viswanathan force-pushed the 3029-eksctl-info branch 2 times, most recently from efd238f to 0e80219 Compare June 24, 2021 03:43
pkg/info/info.go Outdated
Comment on lines 55 to 62
func String() string {
if data, err := json.Marshal(GetInfo()); err == nil {
return string(data)
}
return ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's print something here when the marshal fails, just so users don't get confused about why the command returned nothing:

Suggested change
func String() string {
if data, err := json.Marshal(GetInfo()); err == nil {
return string(data)
}
return ""
}
func String() string {
data, err := json.Marshal(GetInfo())
if err != nil {
return fmt.Sprintf("failed to marshal info into json: %q", err)
}
return string(data)
}

@neha-viswanathan neha-viswanathan changed the title WIP: eksctl info command eksctl info command Jun 25, 2021
@neha-viswanathan neha-viswanathan marked this pull request as ready for review June 25, 2021 02:14
Expect(err).NotTo(HaveOccurred())

oses := []string{"aix", "android", "darwin", "dragonfly", "freebsd", "hurd", "illumos", "ios", "js", "linux", "nacl", "netbsd", "openbsd", "plan9", "solaris", "windows", "zos"}
Expect(result.OS).To(BeElementOf(oses))
Copy link
Contributor

Choose a reason for hiding this comment

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

oooh i didn't know BeElementOf was a thing! nice!

Copy link
Contributor

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 thanks for working on this!

@Callisto13 Callisto13 merged commit e9c8c94 into eksctl-io:main Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eksctl info command
3 participants