-
Notifications
You must be signed in to change notification settings - Fork 130
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-13969 Remove s3.push and s3.pull #8637
base: main
Are you sure you want to change the base?
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.
Great cleanup! Did we re-verify with all-configs or splunk that there isn't usage of these commands anymore?
@@ -70,9 +70,6 @@ type attachResults struct { | |||
func attachResultsFactory() Command { return &attachResults{} } | |||
func (c *attachResults) Name() string { return evergreen.AttachResultsCommandName } | |||
|
|||
// ParseParams decodes the S3 push command parameters that are |
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 we keep this comment and just reword for attach results?
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 was cleaning up the comments since only these commands document this function. I'm okay with documenting it but do you want me to add it to all the other commands?
@@ -100,7 +100,6 @@ type s3Loc struct { | |||
func s3CopyFactory() Command { return &s3copy{} } | |||
func (c *s3copy) Name() string { return "s3Copy.copy" } | |||
|
|||
// ParseParams decodes the S3 push command parameters |
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.
same, keep but s3 copy
@@ -35,10 +35,6 @@ | |||
"admins": ["annie.black"], |
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.
didn't know i was in the codebase this much LOL
// If the user requested task sync in their patch, it should match at least | ||
// one valid task in a build variant. | ||
if len(patchDoc.SyncAtEndOpts.VariantsTasks) == 0 { | ||
j.gitHubError = NoSyncTasksOrVariants |
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.
we can remove NoSyncTasksOrVariants as well
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.
Ty! I still find it ridiculous that the linter official doesn't consider const in blocks like this unused. Removed
Message: fmt.Sprintf("task '%s' not found", rh.taskID), | ||
}) | ||
} | ||
return gimlet.NewTextResponse(t.S3Path(t.BuildVariant, t.DisplayName)) |
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.
We can get rid of S3Path now
validator/project_validator.go
Outdated
var longErrorValidators = []longValidator{ | ||
validateTaskSyncCommands, | ||
} | ||
var longErrorValidators = []longValidator{} |
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.
We can just get rid of this and the --long flag, since there's nothing in here! (Can do in another PR but might be good to do as a quick-win in the nearterm just to keep things clean)
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.
Sure, I wasn't sure if we regularly cycle validators in here. I'll remove it in this PR!
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've removed the long flag from validate command as well. The latest commit has the changes for the long flag/validator
DEVPROD-13969
Description
This removes the s3.push and s3.pull commands.
Testing
Existing unit tests.
Spruce PR
#574 has to be merged first.