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

APP-6785: Remove local control page - remove web workflows #4523

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

ethanlookpotts
Copy link
Contributor

@ethanlookpotts ethanlookpotts commented Nov 4, 2024

We are removing RC from the RDK - users are now expected to use app.viam.com to control their machine.

This PR just removes the workflows and anything web related from .github, which makes it easier to remove the code in the next step. (There is risk that there will be drift & issues in web/frontend until the latter is submitted, but we don't expect any changes there because we're removing it.)

  1. 👉🏼 APP-6785: Remove local control page - remove web workflows #4523
  2. APP-6785: Remove local control page (RC) #4520

Review requests

  • Check .github - did I miss anything related to the web app?
  • Are you the right person to review this PR? Is there someone else that you think should be on the PR?
  • Does this plan work for you?

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 4, 2024
@ethanlookpotts ethanlookpotts marked this pull request as ready for review November 4, 2024 20:26
@ethanlookpotts ethanlookpotts added appimage Build AppImage of PR appimage-ignore-tests Build AppImage of PR and ignore tests static-build Build static binaries from PR static-ignore-tests Build static binaries from PR and ignore tests labels Nov 4, 2024
Copy link
Member

@abe-winter abe-winter left a comment

Choose a reason for hiding this comment

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

lgtm

error cases here are:

  • a workflow fails when you merge this
  • a workflow fails after you merge the next PR

neither one drastic imo, and what you have looks right

Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

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

Seems like there are a few other npm-publish.yml references:

I think this will cause the workflows on main to fail.

➜  workflows git:(APP-6785/ethanlookpotts/remove-web-workflows) ✗ rg "npm"
main.yml
63:  npm_publish:
64:    uses: viamrobotics/rdk/.github/workflows/npm-publish.yml@main

stable.yml
63:  npm_publish:
64:    uses: viamrobotics/rdk/.github/workflows/npm-publish.yml@main

releasecandidate.yml
46:  npm_publish:
47:    uses: viamrobotics/rdk/.github/workflows/npm-publish.yml@main

@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 Nov 4, 2024
@ethanlookpotts
Copy link
Contributor Author

@zaporter-work great catch

9984241

Copy link
Member

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

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

Nothing else stands out -- LGTM

@ethanlookpotts
Copy link
Contributor Author

Ah - didn't realize that James was OOO today - will wait to merge until tomorrow so he has a chance to review.

Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

The workflow removals look fine to me. Bigger question about how this may break the agent's healtchecks (it queries localhost:8080 every minute) but will follow up about that in Slack/elsewhere.

@ethanlookpotts
Copy link
Contributor Author

Holding on this PR until we resolve the discussion about agent healthchecks.

@ethanlookpotts
Copy link
Contributor Author

I was able to verify that #4520 works as expected, so I'm going to go ahead and merge this and put the next PR up for review. Please holler if you notice anything with workflows!

@ethanlookpotts ethanlookpotts merged commit 3af2e89 into main Nov 7, 2024
30 checks passed
@ethanlookpotts ethanlookpotts deleted the APP-6785/ethanlookpotts/remove-web-workflows branch November 7, 2024 22:31
hexbabe pushed a commit to hexbabe/sean-rdk that referenced this pull request Nov 14, 2024
hexbabe pushed a commit to hexbabe/sean-rdk that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appimage Build AppImage of PR appimage-ignore-tests Build AppImage of PR and ignore tests safe to test This pull request is marked safe to test from a trusted zone static-build Build static binaries from PR static-ignore-tests Build static binaries from PR and ignore tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants