-
Notifications
You must be signed in to change notification settings - Fork 129
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
DEVPROD-7896 create stub version if alias not found #8563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
if err != nil { | ||
return v, errors.Wrap(err, "error finding project alias") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the var aliasErr string
to above this check since it's not relevant to the above logic? When I read through the above logic, I expected it to be used and thought it was an error at first but then realized it wasn't
repotracker/repotracker.go
Outdated
@@ -621,11 +621,32 @@ func CreateVersionFromConfig(ctx context.Context, projectInfo *model.ProjectInfo | |||
} | |||
v.Ignored = ignore | |||
|
|||
// Compute aliases first, so we can include this in the version errors if there's a problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this comment say something specific about what we are including in the version errors so future readers don't think the below segment should be adding to project errors rather than returning an error (L630 & L635)?
Maybe like `Compute aliases first to include undefined requested alias in version errors."
units/periodic_builds_test.go
Outdated
@@ -5,6 +5,8 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
"github.com/stretchr/testify/require" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why our lint or formatter allows this (and this comment isn't to fix it, there's probably all around our codebase), but the testify/assert
package is imported down on L17 and then this testify/require
is now it's own block on L8...
DEVPROD-7896
Description
This change creates a stub version with an error if an alias is requested but not found.
The git tag version of the error isn't super human readable but I also think it's worth keeping it to this level of simplicity, since that case shouldn't happen with validation anyway.
Testing
Added a unit test.