feat: add image pull secrets #50
Conversation
## Walkthrough
The Earthfile import of the core Earthly image was changed from a fixed version tag to a parameterized argument variable, allowing external override of the core image version. Additionally, error handling in the `refreshToken` method was enhanced by wrapping discovery errors with contextual messages.
## Changes
| File(s) | Change Summary |
|-------------------------------------|-----------------------------------------------------------------------------------------------------|
| Earthfile | Replaced fixed core Earthly image import with a parameterized argument variable for flexible import |
| internal/client/interceptors/auth_interceptor.go | Wrapped error from `client.Discover` call with contextual message in `refreshToken` method |
## Possibly related PRs
- formancehq/stargate#49: Both PRs modify the Earthfile's import of the core Earthly image, with the main PR parameterizing the import version while the retrieved PR updates the fixed version number.
## Suggested reviewers
- paul-nicolas
## Poem
> A hop and a skip, I tweak with delight,
> The Earthfile now flexes, its v
8000
ersion just right.
> With arguments handy, the image can change—
> My burrow of builds now has wider range!
> 🐇✨✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Earthfile (1)
3-4: Parameterising the core image is a solid improvement – minor polish suggestedThe
ARG→IMPORTpattern is valid and keeps existingcore+…references intact.
Two small tweaks that would improve clarity & usability:
- Rename the build-arg (e.g.
CORE_IMAGE) to avoid the same identifier serving double duty as both a variable ($core) and an alias (core). This eliminates the mental overhead of remembering which namespace you are in.- Add a short comment or README note showing how to override the image when invoking Earthly (
earthly +compile --core=github.com/formancehq/earthly:tags/v0.19.2), so downstream users discover the new capability.No functional blockers – just a readability touch-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Earthfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/client/interceptors/auth_interceptor.go (1)
119-119: Consider consistency in error handling approach.The error wrapping enhancement is good practice and provides better context for debugging. However, this creates inconsistency within the file - line 130 uses
errors.Wrapffrom thegithub.com/pkg/errorspackage while this line usesfmt.Errorf.Consider standardizing on one approach:
Option 1: Use errors.Wrapf for consistency with existing code:
- return time.Time{}, fmt.Errorf("cannot discover endpoint: %w", err) + return time.Time{}, errors.Wrapf(err, "cannot discover endpoint")Option 2: Update line 130 to use fmt.Errorf for consistency:
- return time.Time{}, errors.Wrapf(err, "cannot fetch token") + return time.Time{}, fmt.Errorf("cannot fetch token: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Earthfile(1 hunks)internal/client/interceptors/auth_interceptor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Earthfile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
🔇 Additional comments (2)
internal/client/interceptors/auth_interceptor.go (2)
5-5: LGTM: Import addition supports error wrapping enhancement.The
fmtimport is correctly added to support the error wrapping on line 119.
1-137: PR title doesn't match the actual changes.The PR title mentions "add image pull secrets" but the changes in this file are about error handling improvements in an OAuth2/OIDC authentication interceptor. Please verify if this discrepancy is intentional or if there are missing files in the review.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Bug: Dynamic Dependency Breaks Build Reproducibility
Switching the core dependency import from a specific version tag (v0.19.1) to the main branch breaks build reproducibility. The main branch is a moving target containing development code, leading to non-deterministic and potentially unstable builds.
Earthfile#L2-L4
Lines 2 to 4 in 923e8ee
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.