Added support for builder-jammy-full, and made it default
#2659
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andreas-kupries
added
kind/enhancement
|
Note: rails and wordpress require updates to work ... rails simply goes 404. wordpress in apps, cli-apps does not stage: |
andreas-kupries
force-pushed
the
2153-jammy
branch
from
7b851e1
to
667777d
Compare
Codecov ReportAttention:
... and 18 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
This was referenced Oct 18, 2023
note: dropped hard-coded user/group information. note: staging scripts to use based on chosen builder image (glob matching, details in reference docs) chore: switched to builder-jammy-full as default builder. note: The old bionic based builder is still supported, via a custom set of staging scripts. chore: updated testsuite (wordpress, rails require bionic builder for now) chore: extended action tracing in server and scripts chore: factored assembly of stage env into separate function to avoid `funlen` warning on `newJobRun`.
andreas-kupries
force-pushed
the
2153-jammy
branch
from
162b6f3
to
cbb6b76
Compare
enrichman
requested changes
Oct 19, 2023
There was a problem hiding this comment.
It looks good to me! Pretty nice. I would just make a couple of small refactoring:
since the staging script ConfigMap contains a bit of information it will be probably easier to handle it as a small struct. It a bit error prone to use the Data["xxx"] because you could get some errors with just a typo, and also you have to know/remember the type that you are getting to cast it properly (such the uid/guid).
So I would create something like this with a constructor:
type StagingScriptConfig struct {
UserID int
GroupID int
Base string
DownloadImage string
UnpackImage string
Builder string
BuilderImage string
}
func NewStagingScriptConfig(config *v1.ConfigMap) (*StagingScriptConfig, error) {
stagingScript := &StagingScriptConfig{
Base: config.Data["base"],
// ...
}
groupID, err := strconv.ParseInt(config.Data["groupID"], 10, 64)
if err != nil {
return nil, err
}
stagingScript.GroupID = int(groupID)
// ...
return stagingScript, nil
}and as a personal preference I would also move out the fallback check.
It will do one more loop in case of a missing match, but it's much more readable (and it should be just a rare case). Also performance-wise I don't think it makes any difference.
stagingScripts := []*StagingScriptConfig{}
for _, configmap := range configmapList.Items {
stagingScript, err := NewStagingScriptConfig(configMap)
if err != nil {
return nil, err
}
stagingScripts = append(stagingScripts, stagingScript)
// check for matching staging script
matched, err := filepath.Match(pattern, stagingScript.Builder)
if err != nil {
return nil, err
}
if matched {
logger.Info("locate staging scripts - match", "name", configmap.Name, "for", pattern, "builder", builder)
return stagingScript, nil
}
}
// no matches, find fallback
for _, stagingScript := range stagingScripts {
if stagingScript.Builder == "*" || stagingScript.Builder == "" {
logger.Info("locate staging scripts - using fallback", "name", fallback.Name)
return stagingScript, nil
}
}
return nil, fmt.Errorf("no matches, no fallback")
|
That small comment needs quite a bit of code work ... Still, most of it is ok and will work on that. The search logic of yours however is semantically different. |
…lly. this simplified the resolution of `base` references too. search kept as-is
|
Comment addressed with commit fb2c9b0. |
enrichman
reviewed
Oct 19, 2023
more tracing (full set of specs, explicit note about base/inheritance)
enrichman
approved these changes
Oct 20, 2023
|
Checked in: Check 1
|
andreas-kupries
changed the title
builder-jammy-fullbuilder-jammy-full, and made it default
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ticket #2153
chart epinio/helm-charts#502
docs epinio/docs#356