-
Notifications
You must be signed in to change notification settings - Fork 672
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
reimplement functions in go-mouff-update, use ghreposervice #5470
Conversation
Signed-off-by: novahow <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5470 +/- ##
==========================================
- Coverage 60.97% 60.93% -0.05%
==========================================
Files 794 796 +2
Lines 51488 51640 +152
==========================================
+ Hits 31397 31465 +68
- Misses 17199 17270 +71
- Partials 2892 2905 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for your PR. Can you fix the lint errors? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really have to provide real implementations of all these functions in provider.go?
Also, can you add a few unit tests?
flytectl/pkg/github/provider.go
Outdated
// type GitHubProvider struct { | ||
// provider.Github | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove.
Signed-off-by: novahow <[email protected]>
Currently I haven't came up with a way to better utilize the original provider, since embedding the original provider didn't seem feasible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests.
It's looking good. Just a few comments.
Signed-off-by: novahow <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this change. We're getting there.
Can you re-enable upgrade_test.go
tests by removing this TestMain?
Also, in order to fix the version
subcommand we have to make a slight change to how we pipe the latest flytectl version in flytectl makefile. Change GIT_VERSION
to GIT_VERSION := $(shell git describe --dirty --tags --long --match flytectl/* --first-parent | sed 's/^flytectl\///')
Signed-off-by: novahow <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your work on this!
…#5470) * reimplement functions in go-mouff-update, use ghreposervice Signed-off-by: novahow <[email protected]> * add tests Signed-off-by: novahow <[email protected]> * fix version error Signed-off-by: novahow <[email protected]> * remove testmain Signed-off-by: novahow <[email protected]> * Run make lint-fix Signed-off-by: Eduardo Apolinario <[email protected]> * Call TearDown Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: novahow <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]> Signed-off-by: Vladyslav Libov <[email protected]>
Tracking issue
closes #5372
Why are the changes needed?
flytectl upgrade is not working after monorepo integration
What changes were proposed in this pull request?
Cloned the code from the original external package https://github.com/mouuff/go-rocket-update/blob/v1.5.4/pkg/provider/provider_github.go , and made modifications to fit our needs
How was this patch tested?
make test_unit
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link