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

Add functionality for users to retrieve template by Name as well as UUID #553

Merged
merged 4 commits into from
May 4, 2022
Merged

Add functionality for users to retrieve template by Name as well as UUID #553

merged 4 commits into from
May 4, 2022

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Nov 2, 2021 •
edited
Loading

Description

This PR adds the ability for Tink to retrieve workflows by either UUID or by Name.
If the value after tink template get is not a valid UUID, Tink will default to searching for the value in the Name field of the templates.

Mostly developed by @Belchy06, thanks!
My additions are:

  • cleaned up the import blocks so its just 2 blocks
  • fix typo in help output
  • rebase on latest main
  • rework some of the logic in cmd/tink-cli/cmd/get/get.go to be clearer

Why is this needed

This feature is required for a project @Belchy06 is working on but is also something I'd like to make use of.

How Has This Been Tested?

This has been tested in the VM's based on the Local Setup with Vagrant tutorial, as well as in a personal deployment by @Belchy06.

@mmlb mmlb mentioned this pull request Nov 2, 2021
Closed
3 tasks
@mmlb mmlb changed the title Pull 541 tweaks Add functionality for users to retrieve template by Name as well as UUID Nov 2, 2021
@mmlb mmlb requested review from tstromberg and removed request for tstromberg November 2, 2021 21:44
@mmlb mmlb force-pushed the pull-541-tweaks branch 3 times, most recently from 435357c to 965b4ec Compare November 15, 2021 23:30
@mmlb mmlb marked this pull request as ready for review November 15, 2021 23:32
mmlb
mmlb commented Nov 15, 2021
View reviewed changes
cmd/tink-cli/cmd/get/get.go Outdated
@@ -155,3 +148,31 @@ func NewGetCommand(opt Options) *cobra.Command {
opt.clientConnOpt.SetFlags(cmd.PersistentFlags())
return cmd
}

func retrieveMulti(opt Options, cmd *cobra.Command, args []string) ([]interface{}, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to extract this into a separate function to get golangci-linter happy.

@codecov
Copy link

codecov bot commented Nov 15, 2021 •
edited
Loading

Codecov Report

Merging #553 (15fb537) into main (e184cbc) will increase coverage by 0.28%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   44.37%   44.65%   +0.28%     
==========================================
  Files          61       61              
  Lines        3491     3509      +18     
==========================================
+ Hits         1549     1567      +18     
- Misses       1858     1860       +2     
+ Partials       84       82       -2
Impacted Files Coverage Δ
cmd/tink-cli/cmd/template/get.go 47.45% <31.25%> (-2.55%) ⬇️
cmd/tink-cli/cmd/get/get.go 90.16% <90.00%> (+10.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e184cbc...15fb537. Read the comment docs.

nshalman
nshalman reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@nshalman nshalman left a comment

Choose a reason for hiding this comment

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

This all looks reasonable to me. Any idea why CI is failing, @mmlb?

@jacobweinstock
Copy link
Member

Hey @mmlb, is this still on your radar? Any help needed?

@mmlb
Copy link
Contributor Author

mmlb commented Dec 14, 2021

Hey @mmlb, is this still on your radar? Any help needed?

Still on the radar not sure why I hadn't merged it yet. I think I wanted to give it a go via sandbox but forgot/got distracted with boot issues I was debugging :D. I'll get CI to pass and get it merged soon.

@mmlb
Copy link
Contributor Author

mmlb commented Dec 14, 2021

This all looks reasonable to me. Any idea why CI is failing, @mmlb?

Rebased on top of master and now failing on actions/setup-go not being nice and exporting GOROOT which messes with the go being run within nix-shell. I'm going to open up a PR to actions/setup-go so it stops doing this.

@mmlb
Copy link
Contributor Author

mmlb commented Dec 14, 2021

Waiting on actions/setup-go#175

@swills swills mentioned this pull request Jan 3, 2022
Closed
@mmlb
Copy link
Contributor Author

mmlb commented Feb 8, 2022

I'll rebase on top of main now that #582 should be unblocking this.

@mmlb mmlb self-assigned this Feb 8, 2022
@mmlb mmlb force-pushed the pull-541-tweaks branch 2 times, most recently from 25343c6 to 1c544ec Compare April 18, 2022 21:52
@jacobweinstock
Copy link
Member

jacobweinstock commented May 3, 2022 •
edited
Loading

Hey @mmlb, just a ping on this one. I can take a look after the merge conflicts are resolved.

@jacobweinstock jacobweinstock self-requested a review May 3, 2022 15:29
@mmlb
Copy link
Contributor Author

mmlb commented May 4, 2022

Thanks for the ping @jacobweinstock, lets see how things go.

@mmlb
Copy link
Contributor Author

mmlb commented May 4, 2022

ah the tests are still going to fail though, lets see if thats a quick fix.

@mmlb mmlb force-pushed the pull-541-tweaks branch 2 times, most recently from 66c9012 to 2102da8 Compare May 4, 2022 19:35
@mmlb
Copy link
Contributor Author

mmlb commented May 4, 2022

alright @jacobweinstock I'm all rebased and tests passing!

jacobweinstock
jacobweinstock previously approved these changes May 4, 2022
View reviewed changes
@mmlb mmlb added ready-to-merge Signal to Mergify to merge the PR. and removed ready-to-merge Signal to Mergify to merge the PR. labels May 4, 2022
Belchy06 and others added 2 commits May 4, 2022 16:15
@Belchy06 @mmlb
Add functionality for users to retrieve template by Name as well as UUID
a6e76fb 
Signed-off-by: William Belcher <[email protected]>
Co-Authored-by: Manuel Mendez <[email protected]>
Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
tink-cli: Ensure opt.RetrieveData is non-nil before invoking
15fb537 
Otherwise we get a nice panic.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Copy link
Contributor Author

mmlb commented May 4, 2022

Hey @jacobweinstock ptal again. I added a couple more tests, for when the function that fetches the data doesn't exist. And added some protection to avoid running an undefined function.

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label May 4, 2022
@mmlb mmlb requested a review from jacobweinstock May 4, 2022 20:24
@mergify mergify bot merged commit a0f65e9 into tinkerbell:main May 4, 2022
@mmlb mmlb deleted the pull-541-tweaks branch May 4, 2022 20:35
@Belchy06
Copy link
Contributor

Awesome to see this merged in.

Sorry for going MIA, my studies and work absolutely swamped me and I was unable to set some time aside to get the original PR complete.

Thanks guys!

@mmlb
Copy link
Contributor Author

mmlb commented Jun 1, 2022

@Belchy06 no worries, thanks for kicking this off.

@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nshalman nshalman nshalman left review comments

@jacobweinstock jacobweinstock jacobweinstock approved these changes

Assignees

@mmlb mmlb

Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mmlb @jacobweinstock @Belchy06 @nshalman @displague