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

[RSDK-4975] CLI update warning #3585

Merged
merged 17 commits into from
Feb 21, 2024

Conversation

mattjperez
Copy link
Member

RSDK-4975

On installs older than one week, warns users when their CLI is out of date against the latest stable release.

@mattjperez mattjperez self-assigned this Feb 13, 2024
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Feb 13, 2024
@mattjperez
Copy link
Member Author

This is still a draft, still needs tests and verification. Wanted to get feedback on the approach, messages, etc. before going further.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 14, 2024
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Nice! Some initial comments.

cli/app.go Outdated Show resolved Hide resolved
cli/client.go Outdated Show resolved Hide resolved
cli/client.go Outdated Show resolved Hide resolved
cli/client.go Outdated Show resolved Hide resolved
cli/client.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 14, 2024
@mattjperez
Copy link
Member Author

to test the changes you can alter the following in rdk/Makefile before running make cli

TAG_VERSION?=$(shell echo "v0.0.1") // some version
DATE_COMPILED?=$(shell echo "2021-01-01") // some date

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 14, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 20, 2024
cli/client.go Outdated Show resolved Hide resolved
TAG_VERSION?=$(shell git tag --points-at | sort -Vr | head -n1)
LDFLAGS = -ldflags "-s -w -extld="$(shell pwd)/etc/ld_wrapper.sh" -X 'go.viam.com/rdk/config.Version=${TAG_VERSION}' -X 'go.viam.com/rdk/config.GitRevision=${GIT_REVISION}'"
TAG_VERSION?=$(shell git tag --points-at | sort -Vr | head -n1 | grep . || echo "(dev)")
DATE_COMPILED?=$(shell date +'%Y-%m-%d')
Copy link
Member

@zaporter-work zaporter-work Feb 20, 2024

Choose a reason for hiding this comment

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

[opt] A friend of mine mentioned that using the go debug vcs.time would work as well.

See: https://pkg.go.dev/runtime/debug#BuildSetting
We report this in viam --debug version

Note: if you compile the cli with go build ./viam/main.go the vcs debug info won't be there. golang/go#51279

I don't want to drag down this PR in refactors so I don't see a strong reason to do this now, but might be worth considering in the future.

Copy link
Member

@maximpertsov maximpertsov left a comment

Choose a reason for hiding this comment

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

looks good w/ respect to my change requests

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 20, 2024
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -380,6 +387,129 @@ func RobotsPartShellAction(c *cli.Context) error {
)
}

// checkUpdateResponse holds the values used to hold release information.
Copy link
Member

Choose a reason for hiding this comment

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

nit: rename this too

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 20, 2024
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Nice! LGTM 🧑‍🔧 .

@mattjperez mattjperez merged commit 5ebf64b into viamrobotics:main Feb 21, 2024
16 checks passed
@mattjperez mattjperez deleted the rsdk-4975-cli-upgrade-warning branch February 21, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants