Fallback download URL logic for steps#369
Conversation
| const precompiledStepsEnv = "BITRISE_EXPERIMENT_PRECOMPILED_STEPS" | ||
| const precompiledStepsDefaultStorage = "https://storage.googleapis.com/bitrise-steplib-storage" | ||
| const precompiledStepsPrimaryStorageEnv = "BITRISE_PRECOMPILED_STEPS_PRIMARY_STORAGE" | ||
| const precompiledStepsStorageURLsEnv = "BITRISE_PRECOMPILED_STEPS_STORAGE_URLS" |
There was a problem hiding this comment.
I don't think we'll ever change the URL of the backup location (GCS bucket). I think this diff could be simpler if the backup location was hardcoded into this repo, and only the "override" would be coming from the env var.
There was a problem hiding this comment.
I was mainly thinking about testing the new bucket, where we don't want any fallback options to be used. With this change we could just pass a list with 1 source and it could fail if something is misconfigured.
There was a problem hiding this comment.
Thank you, that's a good point and I agree
| const precompiledStepsDefaultStorage = "https://storage.googleapis.com/bitrise-steplib-storage" | ||
| const precompiledStepsPrimaryStorageEnv = "BITRISE_PRECOMPILED_STEPS_PRIMARY_STORAGE" | ||
| const precompiledStepsStorageURLsEnv = "BITRISE_PRECOMPILED_STEPS_STORAGE_URLS" | ||
| const precompiledStepsPrimaryStorageEnvDeprecated = "BITRISE_PRECOMPILED_STEPS_PRIMARY_STORAGE" |
There was a problem hiding this comment.
It's not used anywhere, we could just kill it. The only external user of these env vars is this: https://github.com/bitrise-io/bitrise-website/blob/54af00872b6a6b690f9ed68945270d0f2436ac2c/components/ci/app/helpers/workflow_helper.rb#L342
| } | ||
|
|
||
| if err != nil { | ||
| log.Warnf("Failed to download from %s: %s\n", url, err) |
There was a problem hiding this comment.
| log.Warnf("Failed to download from %s: %s\n", url, err) | |
| log.Warnf("Failed to download step from %s: %s\n", url, err) |
| { | ||
| name: "With default base URL", | ||
| env: map[string]string{}, | ||
| name: "Override list via env var", |
There was a problem hiding this comment.
nit: there is no env var involved in this test at all, we are testing a different layer
ofalvai
left a comment
ofalvai
left a comment
There was a problem hiding this comment.
Just one more simplification idea, then it's ready to merge!
3 participants
No description provided.