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

[ LOPS-1538 ] Cache the terminus directory and only add token if not already logged in #17

Merged
merged 13 commits into from
Mar 21, 2024

Conversation

stovak
Copy link
Member

@stovak stovak commented Jun 9, 2023

No description provided.

@stovak stovak requested a review from a team as a code owner June 9, 2023 21:27
@stovak stovak marked this pull request as draft June 9, 2023 21:27
@G-Rath
Copy link
Collaborator

G-Rath commented Jun 9, 2023

fwiw while I've not dig into how Terminus sessions actually work, I expect this will break for repositories that use multiple machine tokens because it isn't included in your cache key.

I'm not sure if it's safe to include it in there either, and unfortunately GHA only supports hashFiles so you'd need to write it to a file first to determine its hash

@stovak
Copy link
Member Author

stovak commented Jun 9, 2023

@G-Rath the problem that we're having is people running a bunch of builds and running afoul of our internal API resource limits. How common is using multiple machine tokens for terminus in a CI environment...??? I can't think of a use-case.

Also, yes, you are correct. It will break that.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 9, 2023

With Pantheons current permission model I don't think there is a strong usecase, but I do live in hope it'll be improved at some point to allow us security folk to better lockdown our CIs 😉

But I know that won't be an overnight thing so I mainly mention this for completeness - at this point because of how niche the usecase is it's probably not even worth having an input to allow disabling the cache...

Something else to consider though is the lifetime of the session - am I right in thinking auth:login first checks if a valid session exists already, and only actually hits the API if its expired?

@stovak stovak marked this pull request as ready for review June 12, 2023 17:33
@scottbuscemi scottbuscemi changed the title [ CMSO-1538 ] Cache the terminus directory and only add token if not already logged in [ LOPS-1538 ] Cache the terminus directory and only add token if not already logged in Jan 18, 2024
Kyle Taylor added 11 commits March 7, 2024 15:24
- Update actions/cache to v4
- Add steps for encrypting/decrypting cached session
- Separate step just for caching Terminus plugins
- gpg2 is not installed on MacOS by default, openssl is available
- clean up some comments
Address "deprecated key derivation used" output
- Remove redundant auth step
- Use whoami to validate session
- Remove redundant file copying steps
- Remove Terminus plugins cache (can add back later if we support installing plugins)
@kyletaylored
Copy link
Contributor

kyletaylored commented Mar 8, 2024

I've integrated the source code from Lullabot's GitHub Action (#18) so that their action could be deprecated (talked with @penyaskito yesterday about this).

  1. Cache implemented for sharing session across jobs
  2. Session is encrypted using openssl / aes-256 before being stored into cache
  3. Session is scoped to workflow run (re-running same workflow will use existing cache, new workflows create new session)

You can see a successful run of it here (see embedded steps in primary job and child jobs): https://github.com/the-scranton-branch/Scranton-WordPress/actions/runs/8206546810/job/22454187954

Copy link
Contributor

@kporras07 kporras07 left a comment

Choose a reason for hiding this comment

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

Looks great!

@kyletaylored kyletaylored merged commit 062f232 into main Mar 21, 2024
5 checks passed
@kyletaylored kyletaylored deleted the CMSO-1538 branch March 21, 2024 15:34
@kyletaylored
Copy link
Contributor

This also fixes #18 as it incorporates some of the code natively versus referencing it. Will close that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants