Skip to content
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.

Sign up for GitHub

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

Include ID of get-log file in the post #34

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Include ID of get-log file in the post #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions server/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func (p *Plugin) createEphemeralPost(userID, channelID, message string) {
}

// createPost creates a non epehemeral post
func (p *Plugin) createPost(userID, channelID, message string) {
func (p *Plugin) createPost(userID, channelID, message string, fileIds ...string) {
userInfo, userInfoErr := p.getJenkinsUserInfo(userID)
if userInfoErr != nil {
p.API.LogError("Error fetching Jenkins user details", "err", userInfoErr.Error())
Expand All @@ -194,6 +194,11 @@ func (p *Plugin) createPost(userID, channelID, message string) {
"attachments": []*model.SlackAttachment{slackAttachment},
},
}

if len(fileIds) > 0 {
post.FileIds = append(post.FileIds, fileIds[0])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to consider all the fieldIDs that are delivered, and not only the first one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, it seems like it's not necessary to consider all fileIDs as only one is being delivered. Keeping it this way might clarify the purpose of including the variadic parameter.

Copy link
Contributor

@mickmister mickmister Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same, but since there is no code passing more than one at the moment, I think this makes sense to just assume there is at maximum one id present.

But now that I read this code again, I think len(fileIds) > 0 is a more readable way to check the presence of the file ID. Go implicitly checks if it is nil in the process. @DanielSz50 what do you think about making this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, done.

}

if _, err := p.API.CreatePost(post); err != nil {
p.API.LogError("Could not create a post", "user_id", userID, "err", err.Error())
}
Expand Down Expand Up @@ -471,7 +476,6 @@ func (p *Plugin) createDialogForParameters(userID, triggerID, jobName, channelID
// fetchAndUploadBuildLog fetches console log of the given job and build.
// and uploads the console log as file to Mattermost server.
func (p *Plugin) fetchAndUploadBuildLog(userID, channelID, jobName, buildID string) error {
config := p.API.GetConfig()
build, buildErr := p.getBuild(jobName, userID, buildID)
if buildErr != nil {
return buildErr
Expand All @@ -482,8 +486,8 @@ func (p *Plugin) fetchAndUploadBuildLog(userID, channelID, jobName, buildID stri
return errors.Wrap(fileUploadErr, "Error uploading file")
}

msg := fmt.Sprintf("Console log of the build #%d of the job '%s' : %s", build.GetBuildNumber(), jobName, *config.ServiceSettings.SiteURL+"/api/v4/files/"+fileInfo.Id)
p.createPost(userID, channelID, msg)
msg := fmt.Sprintf("Console log of the build #%d of the job '%s'", build.GetBuildNumber(), jobName)
p.createPost(userID, channelID, msg, fileInfo.Id)
return nil
}

Expand Down