Include ID of get-log file in the post

[DanielSz50]

Summary

• CreatePost() method is modified so it can take fileID as a new parameter without changing its behavior when no fileID was given.
• fetchAndUploadBuildLog() method now includes fileID as a parameter in call for CreatePost().

Ticket Link

Fixes #33

[codecov-commenter]

Codecov Report

Merging #34 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master     #34      +/-   ##
=========================================
- Coverage    6.94%   6.93%   -0.01%     
=========================================
  Files           6       6              
  Lines         734     735       +1     
=========================================
  Hits           51      51              
- Misses        665     666       +1     
  Partials       18      18              

Impacted Files	Coverage Δ
server/plugin.go	4.74% <0.00%> (-0.02%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc14b48...4894ed2. Read the comment docs.

[mickmister]

At first it seemed odd to add the variadic parameter for the fileIDs, but I understand why. All of the other code did not need to be updated, which is 16 calls in total. I think this change is fine until we need to add another parameter to the createPost function, which may be never.

LGTM, thanks @DanielSz50!

[larkox]

LGTM, just a small question.

[larkox]

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

[DanielSz50]

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.

[mickmister]

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?

[DanielSz50]

I think you're right, done.

[mickmister]

@DanielSz50 In general it's better to incrementally add your commits to the PR instead of rebase/force-pushing for a few reasons:

• It's easier to review each change after each partial approval, though it is not as big of a deal here since this PR is very small. For larger PRs, the entire PR must be reviewed again, instead of just verifying the requested changes have been fixed.
• It leaves a good history for each stage that was reviewed.

Thanks again for this incredibly quick fix!

Edit: Also we squash commits when we merge, so there's downside in having several commits in the PR, in that regard.

[DanielSz50]

Thanks for the tips, I'm going to keep that in mind while working on PRs in the future.

[mickmister]

@DHaussermann Test steps:

Before this PR:

• /jenkins get-log (name) should show a link to the file
• MM admin should be able to download the file
• non MM admin should not be able to download the file

After the PR:

• /jenkins get-log (name) should embed the file in the post
• Both admin and non MM admin should be able download

[DHaussermann]

Tested and passed

• /jenkins get log [job name] now creates a post with a file attachment instead of a link.
• File attachment can be downloaded by any user regardless of Mattermost role
• Created minor enhancement suggestion issue to make file names unique Include build number in file name when downloading logs #38
  LGTM!

Thanks @DanielSz50 for this fix!