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

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

Closed
wants to merge 7 commits into from

Conversation

Belchy06
Copy link
Contributor

Signed-off-by: William Belcher william.belcher@hotmail.com

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.

This PR also adds a TemplateClient to the Tink client, allowing other services such as Boots to retrieve templates.

Why is this needed

This feature is required for the project I am working on but I am sure there are many other use cases I'm unaware of. Also, tinkerbell/smee#178

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 our personal deployment

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Belchy06 and others added 6 commits August 25, 2021 11:37
Signed-off-by: William Belcher <william.belcher@hotmail.com>
Signed-off-by: William Belcher <william.belcher@hotmail.com>
Signed-off-by: William Belcher <william.belcher@hotmail.com>
@Belchy06
Copy link
Contributor Author

@tstromberg I have once again accidentally closed my last PR... Doh!!

Here is a new one with the minor tweaks you requested. Let me know if there's anything else you'd like touched up!

@jacobweinstock
Copy link
Member

Hey @Belchy06, thanks for submitting this! Looks like it just needs some checks to be looked at.

@jacobweinstock jacobweinstock added kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Sep 21, 2021
Signed-off-by: William Belcher <william.belcher@hotmail.com>
@mmlb mmlb mentioned this pull request Oct 4, 2021
12 tasks
@jacobweinstock
Copy link
Member

Hey @Belchy06, looks like a couple of checks and a merge conflict need attention. Anything I can do to help with this?

@Belchy06
Copy link
Contributor Author

Belchy06 commented Oct 7, 2021

@jacobweinstock Sorry Jacob.

I'm just in the middle of my university studies and have a few weeks of the semester left before I can commit some time to working on this again.

I'm more than happy to put some time into it, just a matter of getting through the semester first.

mergify bot added a commit that referenced this pull request Oct 13, 2021
Lots of fixes to stuff that is dynamically generated along with a few extra bits here and there in related code to make things better.

- [x] Fixes the `go generate` calls
- [x] Changes protobuf/grpc stuff to use buf for both dependency management and building/calling protoc
- [x] Changes from goimports to gofumpt as we want this anyway
- [x] Build the tools in tools.go via the Makefile
- [x] Dropped protoc-gen-doc support as that seemed unused. (easy to bring back when we have a use for it)
- [x] Add a make target to check that the generated files are up to date
- [x] Prettier-ify the GHA yaml files too (not just *.yml files)
- [x] Update README/docs
- [x] Setup CI to ensure files are up to date

<!--- Please describe what this PR is going to change -->

Mostly fixes for dynamically generated files.

<!--- Link to issue you have raised -->

I noticed that #541 had some protobuf format commits which I was not expecting being done separately. I went looking into it and noticed the mess we're in.

## How Has This Been Tested?

CI

## How are existing users impacted? What migration steps/scripts do we need?

Will have better automation for messing with generated files.

## Checklist:

I have:

- [x] updated the documentation and/or roadmap (if required)
- [x] added unit or e2e tests
- [ ] provided instructions on how to upgrade
@mmlb
Copy link
Contributor

mmlb commented Oct 13, 2021

Hey @Belchy06 can you rebase on top of latest main, I've done a bunch of work fixing the generators so the edit you made to template.pb.go should be unnecessary.

@mmlb
Copy link
Contributor

mmlb commented Oct 19, 2021

@Belchy06 I've rebased on top of main and tweaked your PR a little further. Can you verify that "Allow edits from maintainers" is checked? I'd like to update with my changes in pull-541-tweaks. I still get confused whether or not that will actually work with organization type repos. If not I'll just open up a separate PR (4th time's the charm!)

@nshalman
Copy link
Member

nshalman commented Nov 2, 2021

@Belchy06 I've rebased on top of main and tweaked your PR a little further. Can you verify that "Allow edits from maintainers" is checked? I'd like to update with my changes in pull-541-tweaks. I still get confused whether or not that will actually work with organization type repos. If not I'll just open up a separate PR (4th time's the charm!)

@Belchy06 is this something you're still working on?

@mmlb
Copy link
Contributor

mmlb commented Nov 2, 2021

I'd say lets close this and I'll PR my branch that is based on this one.

@mmlb
Copy link
Contributor

mmlb commented Nov 2, 2021

I've opened up a draft PR (#553) that should replace this one. It's still a draft because I have not tested it myself.

@nshalman
Copy link
Member

nshalman commented Nov 3, 2021

Closing in favor of #553. Thank you for the initial work, @Belchy06!

@nshalman nshalman closed this Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants