-
Notifications
You must be signed in to change notification settings - Fork 123
APP-8173: Make local testing for viam apps easier #5036
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
base: main
Are you sure you want to change the base?
Conversation
cli/local_viam_apps_setup.go
Outdated
@@ -0,0 +1,82 @@ | |||
package cli |
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.
a lot of this file is autogenerated by cursor so keep 👀 for stupid stuff
cli/local_viam_apps_test.html
Outdated
@@ -0,0 +1,470 @@ | |||
<!DOCTYPE html> |
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.
same here. lots of vibe coding
cli/local_viam_apps_test.html
Outdated
const hostnameString = document.getElementById('hostname').value; | ||
const machineCookie = JSON.stringify({id: apiKeyId, key: apiKey, hostname: hostnameString}); | ||
const machineIdCookie = `${machineIdString}=${machineCookie}; path=/; domain=localhost; SameSite=Lax`; | ||
const currentMachineCookie = `currentMachine=${machineIdString}; path=/; domain=localhost; SameSite=Lax`; |
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.
NOTE FOR MANUAL TESTING: this assumes your local application is using the currentMachine
cookie for testing. might have to update your application to pull that instead of the machine id from the URL
@bashar-515 this is ready for manual testing! will move it out of "draft" mode once youve confirmed with manually testing an app of yours that this works as expected |
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.
So does this mean that in development I should NOT look to the path to retrieve either the hostname or machine ID but then in prod I should? Is there any way we can avoid making the developer have distinct code for different environments?
@bashar-515 thanks for the feedback! something was wrong in my localapp that was causing this to work before when it shouldn't have. try again! will move out of draft mode once you've had a chance to test this and verify it works. then we can get into cleaning up the code :) |
this is exactly how Gambit is set up today and I filed a ticket in their thing to align the experiences, full circle |
@@ -2479,6 +2479,24 @@ Note: There is no progress meter while copying is in progress. | |||
UsageText: createUsageText("module", nil, false, true), | |||
HideHelpCommand: true, | |||
Subcommands: []*cli.Command{ | |||
{ | |||
Name: "local-app-testing", |
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.
[nit] subcommand seems longer than necessary? e.g. local-test
Something I'm not understanding but want to make sure to bring up now since the CLI is shipped software and we can't hit "undo" with releases the way we can with our cloud software. [q] What is the advantage of adding support for this UI versus a local testing environment that supports integrating into a Viam auth app? Reading from the Viam auth cookie is one of the main integration points for anyone building out a Viam App so I feel that local testing isn't all too useful without that support. |
does gambit use the auth cookie and not the machine credentials? i dont think we expose any documentation or guarantees about how were storing that information etc. |
Sorry I don't understand this question. I'll try to rephrase to see if we can get clarity. We just cut Gambit over to extracting the API key, machine ID, and hostname stored in the cookie -- that was the message I sent over Slack. What I'm saying is that -- because they are unable to support authenticating on localhost, they instead display a page to pop in an API key ID and auth entity (key string). This has the unfortunate consequence of making it impossible to test auth changes outside of a cloud environment for them. |
replaces the part of the Gambit onboarding flow that asks you to do this:
|
&cli.StringFlag{ | ||
Name: "app-url", | ||
Usage: "url where local app is running", | ||
Required: true, | ||
}, |
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.
[q] why do we need this? why isn't it just localhost?
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.
feedback from sync emily call: can we make them both port otherwise it seems like theyre more connected
apiKey: document.getElementById('apiKey').value.trim(), | ||
apiKeyId: document.getElementById('apiKeyId').value.trim(), |
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.
should this maybe be nested to follow the same schema as the auth cookie?
tl;dr from huddle is that this code will get moved out of RDK and into Viam Apps so folks can import as a library, better versioning, and allow us to be more careful with additive features |
@bashar-515 actually, no need to look at this yet. i spoke to @ohEmily and our biggest takeaway is that we probably should have most of this code live alongside viam apps and not in the CLI. then we can import the shared code and things wont get as out of sync |
APP-8173
Work done
viam module local-app-testing
Things this doesnt do
Ask for reviewers
Can you please manually test with a viam app that you have? Will require the following steps:
go build -o ~/go/bin/viam cli/viam/main.go
go run cli/viam/main.go module local-app-testing