- 
                Notifications
    
You must be signed in to change notification settings  - Fork 22
 
feat: use pinned aria-at commit with cron to update #1510
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: development
Are you sure you want to change the base?
Conversation
Test Pinned aria-at version
update pinned version
Ensure that automatic updates happen
| 
           @stalgiag This direction looks great and matches what I think we all agreed on at a previous architecture meeting! I think you can take out of draft and finalize when you're ready.  | 
    
| 
               | 
          ||
| - The pinned SHA lives at `config/aria-at.version`. | ||
| - Local imports without `-c` will read this file (or `ARIA_AT_PINNED_SHA`). | ||
| - CI imports historical commits for coverage and then the pinned commit. | 
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.
This is an aside, but it's still unclear to me why exactly we need those historical commits (and now in addition to the pinned commit)... Is this some tech debt that we could also clean up before next year? cc @howard-e
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.
@stalgia thanks for pushing the work forward on this! Left comments below concerning how this operates in the deployed environments
| - It is recommended to install node with [`nvm`](https://github.com/nvm-sh/nvm) | ||
| 2. Yarn | ||
| - Yarn is resposible for installing dependencies, similar to npm. This project is utilizing yarn workspaces to organize the code into a monorepo structure. | ||
| - Yarn is responsible for installing dependencies, similar to npm. This project is utilizing yarn workspaces to organize the code into a monorepo structure. | 
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.
👍🏽
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.
Wouldn't this change prevent the deployed environments from getting the latest version changes from aria-at? The aria-at.version is effectively static there.
Seems a cron in the deployed environment too or some exclusion of aria-at.version in those environments' deploys with the fail condition relaxed is needed here.
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.
Good point. Maybe we use the aria-at.version from the github repo development as the canonical version and this is what is used during the deploy's "refresh" cron jobs?
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.
That sounds practical to me!
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.
Curious if this meets your concerns 5c44b6e? It is difficult to test this but I think this should work for the cron. Let me know if you have any ideas on how I could test this
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.
I think that this default path should work for enforcing the pinned commit on initial deploy but admittedly Ansible is unfamiliar to me
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.
Yep, these changes look good! After resolving the merge conflicts and reviewing my latest comment, we'd be good to move this forward
| 
               | 
          ||
| on: | ||
| schedule: | ||
| - cron: '0 6 * * *' | 
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.
I worry that this daily interval is too big given how often the deployed environment actually checks (and may update). Right now that's every 15mins. Could we match that?
Overview
This PR does the following
developmentwith the updated pinned SHA and snapshotsThis would resolve many issues with snapshots and it would also catch upstream breaking changes.
Leaving in DRAFT until the overall approach is agreed upon. There is likely some additional consideration needed for how this is handled in deployed environments.
Example workflow runs