MM-21886 - Update matterbuild cutplugin command to split plugin binaries, sign and upload to s3 #17
Conversation
alifarooq0
changed the title
alifarooq0
changed the title
| "PluginSigningSSHUser": "---", | ||
| "PluginSigningSSHHost": "---", |
There was a problem hiding this comment.
Preferably I want to set real values here so whoever wants to tests this locally doesn't have to figure out what goes here. Any concerns if I include the real plugin signing ssh host/username values here @DSchalla ?
There was a problem hiding this comment.
@ali-farooq0 Users will have no access to the server in the future, so I don't see a particular benefit of sharing this in a public repository - The SSH access was meant as a temporary solution for users. What you think?
There was a problem hiding this comment.
Ahh good point. Alright will leave this the way it is then. Thanks @DSchalla!
| 2. Get AWS [Vault](https://developers.mattermost.com/internal/infrastructure/vault/) credentials | ||
| 3. Signed public certificate by Vault |
|
Adding @hanzei from toolkit's perspective. |
|
@ali-farooq0 |
|
@jaydeland Those are needed for the unit tests. |
server/plugin_release.go
Outdated
| if err != nil { | ||
| return err | ||
| return nil, fmt.Errorf("failed to get release by tag err=%w", err) |
There was a problem hiding this comment.
Why don't we use mlog here, or honestly, because I do not know, what is the de facto standard?
There was a problem hiding this comment.
mlog to just log and not return an error? standard way is to use errors pkg or fmt.Errorf()
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
server/plugin_release.go
Outdated
| LogInfo("Will sign the artifact") | ||
| sshClient, err := sshwrapper.DefaultSshApiSetup(Cfg.PluginSigningSSHHost, 22, Cfg.PluginSigningSSHUser, Cfg.PluginSigningSSHKeyPath) | ||
| func getPluginSigningSftpClient(cfg *MatterbuildConfig) (*sftp.Client, error) { | ||
| clientConfig, err := privateKey(cfg.PluginSigningSSHUser, cfg.PluginSigningSSHKeyPath, cfg.PluginSigningSSHPublicCertPath, ssh.InsecureIgnoreHostKey()) |
There was a problem hiding this comment.
I can help with adding the ssh key to the known_hosts file.
| func uploadSignedArtifcatsToS3(fileToUpload []string, tempFolder string) error { | ||
| LogInfo("Uploading signed assets to S3") | ||
| // archiveContains returns filenames that matches a given string. | ||
| func archiveContains(filePath string, contains string) ([]string, error) { |
There was a problem hiding this comment.
Should we write a test for this or is it too much?
server/plugin_release.go
Outdated
| refs, _, err := client.Git.GetRefs(ctx, owner, repository, fmt.Sprintf("tags/%s", tag)) | ||
| if err != nil { | ||
| var gerr *github.ErrorResponse | ||
| if !errors.As(err, &gerr) || gerr.Response.StatusCode != http.StatusNotFound { |
There was a problem hiding this comment.
Nit: I find this hard to read and understand. Maybe an else would make it more clear:
if errors.As(err, &gerr) && gerr.Response.StatusCode == http.StatusNotFound {
LogInfo("tag %s was not found, creating tag", tag)
} else {
return errors.Wrapf(err, "failed to get github tag")
}
server/plugin_release.go
Outdated
| if err != nil { | ||
| LogError("Error opening the file. err=" + err.Error()) | ||
| return err | ||
| // Couldn't achieve gzip level compressions with golang archive api, using shell cmds instead. |
There was a problem hiding this comment.
Thanks for the explanation. How many MBs are saved with using exec? I'm wondering if the cost is worth the risk.
| LogInfo("Will download the github release asset") | ||
| url := strings.Split(assetURL, "/") | ||
| filename := url[len(url)-1] | ||
| func downloadAsset(ctx context.Context, asset *github.ReleaseAsset, folder string) (filePath string, err error) { |
There was a problem hiding this comment.
Not blocking: I think a godoc comment should make clear what the semantic of the return values is. Named return parameters ist offer to much possibilities for bug and bad readable code.
server/plugin_release.go
Outdated
| func cutPlugin(ctx context.Context, cfg *MatterbuildConfig, client *github.Client, owner, repositoryName, tag string) error { | ||
| pluginAsset, err := getPluginAsset(ctx, client, owner, repositoryName, tag) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get plugin asset") |
There was a problem hiding this comment.
Not block: Wrapf is not strictly needed. errors.Wrap is enough. Feel free to change it in the whole file if you want.
| return errors.Wrapf(err, "failed to get plugin asset") | |
| return errors.Wrap(err, "failed to get plugin asset") |
hanzei
added
3: Reviews Complete
* updating vendor dir and mod files * Rework cutplugin command to upload signed platform specific plugin tars to s3 * Confirmed tests are breaking build * Updated waiting text message * Fixed govet * Making check-style happy * Added govendor target * Updated tag created message * Include release link in the success message * PR Feedback * More PR feedback * Creating github client once and passing it around * Creating ctx once and passing it around * inverted some conditions for better readability * using errors.Is * removing vscode settings file * Making govet happy * added PluginSigningSSHHostPublicKey * Fixed Wrapf/Wrap * Updated docs for downloadAsset * Reversed gerr if check * Using piping lib as opposed to bash * Updating readme to include PluginSigningSSHHostPublicKey
Summary
cutPluginto usesftpas opposed toscp, as thego-scpclient was not reliably fetching files from the remote-server (was writing extra bytes to the files).tar.gzfiles, as using Golang's archive API wasn't achieving the same level of compression.--forceflag tocutPluginthat re-signs and uploads to github+s3./matterbuild cutPlugin --tag v0.4.1 --repo mattermost-plugin-demo --forceplugin_release; (not many, but best i could).Makefileto includerun-serverconfig.jsonfor local developmentNOTE Majority of the changes are from the vendor directory, therefore ignore
vendor/*files :).Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-21886