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

ci: upgraded ruby/setup-ruby action from version v1.152.0 to v1.178.0 #233

Merged
merged 9 commits into from
Jul 2, 2024

Conversation

desusai7
Copy link
Contributor

@desusai7 desusai7 commented Jun 20, 2024

📋 Changes

  • upgraded ruby/setup-ruby action from version v1.152.0 to v1.178.0 to support ruby version 3.0.7
  • Added codecov token as a secret to workflows to get rid of rate limiting error.

📎 References

https://github.com/auth0/JWTDecode.swift/actions/runs/9598725630/job/26470757827

@desusai7 desusai7 requested a review from a team as a code owner June 20, 2024 13:58
@desusai7 desusai7 self-assigned this Jun 20, 2024
Widcket
Widcket previously approved these changes Jun 20, 2024
environment: ${{ github.event.pull_request.head.repo.fork && 'external' || 'internal' }}
runs-on: ubuntu-latest
steps:
- run: true
Copy link
Member

@frederikprijck frederikprijck Jun 20, 2024

Choose a reason for hiding this comment

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

The authorize step is required when u use pull_request_target in the on on line 3. If u use pull_request, authorize has no purpose.

You should ask your self:

  • should codecov work for forks?
    • if yes => use pull_request_target and authorize
    • if no => keep pull_request and drop authorize.

pull_request does not expose secrets to forks, meaning it would fail as there are no token found. pull_request_target does expose tokens to secrets, so we need to protect it with authorization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @frederikprijck,

Thank you very much for the detailed explanation on the differences between the triggers pull_request and pull_request_target.

We would like to have codecov working for forks as well but it needs to be authorized first, hence proceeded with pull_request_target.

Pushed the changes accordingly, please take a look again

Copy link
Contributor Author

@desusai7 desusai7 Jul 2, 2024

Choose a reason for hiding this comment

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

On further discussion with @frederikprijck, we've decided it's okay to skip the code coverage check for the PR's raised from forks, as we will have a safety net for this when the PR is raised to master before release, hence changing it back to pull_request and dropping authorize

@desusai7 desusai7 force-pushed the ci/upgrade_setup_ruby_action branch from 293987b to 1ffc886 Compare June 25, 2024 05:53
@Widcket Widcket closed this Jun 25, 2024
@Widcket Widcket reopened this Jun 25, 2024
@desusai7 desusai7 enabled auto-merge July 2, 2024 13:32
@@ -14,7 +14,7 @@ concurrency:

jobs:
test:
name: Test on ${{ matrix.platform.os }} using Xcode ${{ matrix.xcode }}
name: Test on ${{ matrix.platform.os }} using Xcode ${{ matrix.xcode }}s
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: Test on ${{ matrix.platform.os }} using Xcode ${{ matrix.xcode }}s
name: Test on ${{ matrix.platform.os }} using Xcode ${{ matrix.xcode }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

README.md Outdated
@@ -76,7 +76,7 @@ Then, run `carthage bootstrap --use-xcframeworks`.
import JWTDecode
```

2. Decode the token
2. Decode the token using the code snippet shared below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this added for CI purposes or is it an intended change?

Copy link
Contributor

Choose a reason for hiding this comment

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

(for triggering a build, I mean)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will revert them

@desusai7 desusai7 requested a review from Widcket July 2, 2024 14:30
Widcket
Widcket previously approved these changes Jul 2, 2024
@Widcket
Copy link
Contributor

Widcket commented Jul 2, 2024

For context, workflows are disabled for PRs from forks and have to be manually approved: Screenshot 2024-07-02 at 15 31 42

Widcket
Widcket previously approved these changes Jul 2, 2024
@@ -15,6 +15,7 @@ concurrency:
jobs:
test:
name: Test on ${{ matrix.platform.os }} using Xcode ${{ matrix.xcode }}
environment: ${{ github.event.pull_request.head.repo.fork && 'external' || 'internal' }}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added it previously, when we have introduced two different environments for secrets as part of using the authorize step.

Removed it now, as we are no longer using authorize, please take a look again.

@desusai7 desusai7 merged commit 23372dd into master Jul 2, 2024
12 checks passed
@desusai7 desusai7 deleted the ci/upgrade_setup_ruby_action branch July 2, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:tiny Tiny review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants