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 showMessage support to xpls and new version notifications #134

Merged
merged 2 commits into from
Jan 22, 2022

Conversation

tnthornton
Copy link
Member

Description of your changes

Add support to xpls for emitting messages to the end user through window/showMessage. For now, this is only used to inform the end user that a new version is available to download up.

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Beyond the unit tests, I hardcoded my version locally to "v0.5.0" and confirmed that I received the following pop-up:
Screen Shot 2022-01-21 at 4 04 25 PM

- emits a message to the client informing them that a new version is available of up

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
func (i *Informer) newAvailable(local, remote string) bool {
lv, err := semver.NewVersion(local)
if err != nil {
// invalid remote version detected
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// invalid remote version detected
// invalid local version detected

should this be in error message?


"github.com/crossplane/crossplane-runtime/pkg/logging"

"github.com/upbound/up/internal/version"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be bundled into the existing version pkg? I could see this being useful for checking for a new available version in up -v :) (perhaps it should be an optional flag though so that it doesn't make an unexpected network call -- could explore in the future)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing 👍 (I'll move it). I'll hold off on adding it to up --version for now though.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM 👍🏻

moved folded local/remote version comments into errors returned to client

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +47 to +49
type client interface {
Do(*http.Request) (*http.Response, error)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably wouldn't hurt for us to have a small http package eventually as we are probably doing (or will do) some of this in a variety of places 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally agree.

@tnthornton tnthornton merged commit c08714d into upbound:main Jan 22, 2022
@tnthornton tnthornton deleted the megaphone branch January 22, 2022 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants