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-9452 - require logged in status for module generation #4724

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Jan 17, 2025

The current state of the CLI is, for all* commands except for viam module generate, to error out pretty much immediately if the user is not logged in, with a "please run viam login" message. viam module generate, however, will do much of the work happily even if not logged in (e.g., it will run all the user prompting and take in inputs), and then only at the last moment will it try to log in. The behavior after login is inconsistent, however. While I have had no problems with the job successfully logging in and completing, @felixreichenbach, @stevebriskin, and @npentrel have all had issues where the job then failed and had to be rerun. This is a frustrating case because there is non-trivial time and work done in the steps leading up to that failure (e.g., filling out all the user prompts) that all needs to be repeated.

As a first pass solution, this PR brings viam module generate in line with other CLI actions by immediately checking if we're logged in and returning an error if not. As a follow-up, we have a ticket to revisit the ensureLoggedIn logic to try to create more generalized consistency in the direction of not erroring out when possible but instead trying to log in and then proceed with the requested action.

*I haven't tested everything so I might be mistaken when I say all, but at any rate it's very close to all.

@stuqdog stuqdog requested review from a team, purplenicole730 and lia-viam and removed request for a team January 17, 2025 15:39
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 17, 2025
@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 Jan 17, 2025
Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

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

I didn't even realize this was a problem because I always used the flags. Thanks for catching this and fixing it, and organizing efforts for the future work as well!

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.

3 participants