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

Setup strauss installar script to auto download and use latest release #7311

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

ravinderk
Copy link
Collaborator

@ravinderk ravinderk commented Mar 14, 2024

Resolves GIVE-2056

Description

This pull request sets a bash script to automatically use the latest release of strauss package.

Testing Instructions

  • You should be able to successfully run composer install
  • strauss.phar should install when
    • bin/strauss.phar does not exist
    • bin/strauss-version.txt does not exist
    • staruss version in bin/strauss-version.txt does not match latest release.
  • composer install should always install latest release of strauss

Note: you can run bin/strauss-install.sh for testing.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@ravinderk ravinderk requested a review from jonwaldstein March 14, 2024 13:02
@ravinderk ravinderk self-assigned this Mar 14, 2024
@ravinderk ravinderk marked this pull request as ready for review March 14, 2024 13:02
@jonwaldstein
Copy link
Contributor

@ravinderk i'm hesitant to always install the latest release of strauss without testing incremental versions in our workflow.
Is there any reason you would be confident enough to let that automation happen to receive the latest version or do we think there are possibilities of side effects that could go unnoticed without proper testing of each version?

@ravinderk ravinderk changed the title Aetup strauss installar script to auto download and use latest release Setup strauss installar script to auto download and use latest release Mar 16, 2024
@ravinderk
Copy link
Collaborator Author

@ravinderk i'm hesitant to always install the latest release of strauss without testing incremental versions in our workflow. Is there any reason you would be confident enough to let that automation happen to receive the latest version or do we think there are possibilities of side effects that could go unnoticed without proper testing of each version?

@jonwaldstein I locked release version to 0.16.0:7ad261c, that means bash script will only install specific version and clear cached version from bash automatically.

@jonwaldstein
Copy link
Contributor

jonwaldstein commented Mar 26, 2024

@ravinderk okay, thanks that makes sense 😄

@JasonTheAdams I would appreciate your eyes on this as well - since you were the one who setup strauss 🧙.

@JasonTheAdams
Copy link
Contributor

It seems like this is taking what's currently a single line of inline bash and breaking it out into a bash script. Is the current line not doing something? Why not just change the inline script to be https://github.com/BrianHenryIE/strauss/releases/download/0.16.0/strauss.phar?

@ravinderk
Copy link
Collaborator Author

It seems like this is taking what's currently a single line of inline bash and breaking it out into a bash script. Is the current line not doing something? Why not just change the inline script to be https://github.com/BrianHenryIE/strauss/releases/download/0.16.0/strauss.phar?

@JasonTheAdams I added an extra feature in the bash script. The cached bin/strauss.phar will be replaced in bin/ if the current version does not match. That means we will have the same version across team.

@JasonTheAdams
Copy link
Contributor

Got it, @ravinderk! I see the value and I like it!

Rather than storing the version in a file, I suggest using php strauss.phar --version, which will output something like:

strauss 0.14.0

You can use a little regex to easily parse the version from that line. As a note, I put in a request for a plain version flag.

@ravinderk
Copy link
Collaborator Author

Got it, @ravinderk! I see the value and I like it!

Rather than storing the version in a file, I suggest using php strauss.phar --version, which will output something like:

strauss 0.14.0

You can use a little regex to easily parse the version from that line. As a note, I put in a request for a plain version flag.

@JasonTheAdams version command output is not reliable at this moment and I created an issue on their repository BrianHenryIE/strauss#90. For this reason, I am using a txt file to store the version.

@JasonTheAdams
Copy link
Contributor

@ravinderk According to that Issue this is working in the PHAR. I just tested it in 0.14.0 and it worked fine, too.

@ravinderk
Copy link
Collaborator Author

@ravinderk According to that Issue this is working in the PHAR. I just tested it in 0.14.0 and it worked fine, too.

@JasonTheAdams I refactored logic to use version to get strauss library version- c942413

Copy link
Contributor

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

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

Thanks! I like it! I can definitely see this avoiding some strange issues in the future. 👍

@jonwaldstein
Copy link
Contributor

@jonwaldstein
Copy link
Contributor

The github action for generating a zip is failing on this PR due to some permission issue with the vendor-prefixed directory https://github.com/impress-org/givewp/actions/runs/8524271225/job/23348497486

Anyone have an idea what's going on?

@jonwaldstein
Copy link
Contributor

The github action for generating a zip is failing on this PR due to some permission issue with the vendor-prefixed directory https://github.com/impress-org/givewp/actions/runs/8524271225/job/23348497486

Anyone have an idea what's going on?

@JasonTheAdams @ravinderk we have a permission issue with the vendor-prefixed folder ☝️

@ravinderk
Copy link
Collaborator Author

@jonwaldstein I am not sure what is an issue with the rsync command and directory permission. We need to debug the GitHub action, or we can update the workflow to set the correct permission to the vendor-prefixed directory.

Can you take of this?

cc @JasonTheAdams

@jonwaldstein
Copy link
Contributor

@ravinderk yeah, i'm confused why it's only failing from permissions issues in this PR though

Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

The plugin zip is still failing for this which should be resolved before merging.

https://github.com/impress-org/givewp/actions/runs/9682069602/job/26714974478

Screenshot 2024-06-26 at 11 22 59 AM

Copy link

This PR is stale because it has been open 45 days with no activity. Stale PRs will NOT be automatically closed.

@github-actions github-actions bot added the Stale label Aug 11, 2024
@github-actions github-actions bot removed the Stale label Dec 10, 2024
@jonwaldstein
Copy link
Contributor

jonwaldstein commented Dec 10, 2024

Apparently Strauss 0.16.0 introduced some permission issue BrianHenryIE/strauss#104 and was fixed in 0.19.2. We can try updating to the latest at time of writing this which would be 0.20.0

@jonwaldstein
Copy link
Contributor

I attempted to update to 0.20.0 but now we have another permission issue:

Screenshot 2024-12-10 at 4 35 59 PM

@jonwaldstein jonwaldstein self-requested a review December 12, 2024 20:34
Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

Updated to 0.20.1 with the permissions fix from @defunctl and we're back in business!

https://github.com/impress-org/givewp/actions/runs/12304079021

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.

3 participants