Skip to content

Comments

Make etc/ci/upload_nightly.sh responsible for putting datetimes into filenames#12163

Closed
imjacobclark wants to merge 5 commits intoservo:masterfrom
imjacobclark:12128-uploadnightlysh-responsible-date-times
Closed

Make etc/ci/upload_nightly.sh responsible for putting datetimes into filenames#12163
imjacobclark wants to merge 5 commits intoservo:masterfrom
imjacobclark:12128-uploadnightlysh-responsible-date-times

Conversation

@imjacobclark
Copy link
Contributor

@imjacobclark imjacobclark commented Jul 2, 2016 •
edited
Loading

This is part of #11980, allowing for more reproducible builds, increasing the security and integrity of the release process.

This particular change ensures the date and time is on the files that are available for download from the nightly-release s3 bucket and removes that responsibility from the packager.

See #12128 for more details.

@aneeshusa

  • There are tests for these changes OR
  • These changes do not require tests because they are part of the CI infrastructure

This change is Reviewable

@highfive
Copy link

highfive commented Jul 2, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/package_commands.py
  • @aneeshusa: etc/ci/upload_nightly.sh

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 2, 2016
@imjacobclark imjacobclark mentioned this pull request Jul 2, 2016
Closed
@imjacobclark imjacobclark force-pushed the 12128-uploadnightlysh-responsible-date-times branch from 534fea9 to 4d966ee Compare July 2, 2016 09:59
@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2016

☔ The latest upstream changes (presumably #12156) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jul 2, 2016
@aneeshusa
Copy link
Contributor

aneeshusa commented Jul 3, 2016 •
edited
Loading

This should also incorporate the change in #12088/wait for that to merge.

aneeshusa
aneeshusa reviewed Jul 3, 2016
View reviewed changes
etc/ci/upload_nightly.sh Outdated

upload() {
local package_filename
local package_filename current_date_time
Copy link
Contributor

@aneeshusa aneeshusa Jul 3, 2016

Choose a reason for hiding this comment

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

Let's rename current_date_time to nightly_timestamp.

Copy link
Contributor

@aneeshusa aneeshusa Jul 3, 2016

Choose a reason for hiding this comment

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

Also rename package_filename to nightly_filename since it'll include the timestamp per my other comment.

@aneeshusa aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 3, 2016
@aneeshusa aneeshusa assigned aneeshusa and unassigned SimonSapin Jul 3, 2016
@imjacobclark
Merge branch 'master' of https://github.com/servo/servo into 12128-up…
834a6a5 
…loadnightlysh-responsible-date-times

Conflicts:
	etc/ci/upload_nightly.sh
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 3, 2016
@imjacobclark
Copy link
Contributor Author

imjacobclark commented Jul 3, 2016 •
edited
Loading

@aneeshusa Resolve the merge conflict and review comments, just need to wait on #12088 landing.

Is #12088 still relevant in that file if this PR moves the time/date logic into upload_nightly.sh?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2016

☔ The latest upstream changes (presumably #12088) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jul 3, 2016
@aneeshusa
Copy link
Contributor

aneeshusa commented Jul 3, 2016

Well, it just merged, so that's a moot point now :)

That PR changes the colons in the timestamp to dashes because the tar command will interpret the filename with colons as containing a remote server address with the colons and try to connect out, instead of knowing that it is a local file. I wish tar would just get smarter, but in the meantime we're switching to dashes so tar doesn't get confused. Any replacement for that functionality also needs to incorporate that change.

@imjacobclark
Copy link
Contributor Author

imjacobclark commented Jul 6, 2016

@aneeshusa I will port the functionality from the Python into upload_nightly.sh, there seems to be a few conflicts on this PR too which I'll resolve shortly.

@aneeshusa aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 18, 2016
@aneeshusa aneeshusa removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Aug 10, 2016
@aneeshusa
Copy link
Contributor

aneeshusa commented Aug 10, 2016

#12088 merged so this is no longer blocked.

@imjacobclark are you still working on this? Do you have any questions?

@imjacobclark
Copy link
Contributor Author

imjacobclark commented Aug 10, 2016

@aneeshusa still working on this, had a bit of a hectic few weeks, however will pick this up again now. Pretty straightforward, I'll catch up on IRC if there are any issues.

@KiChjang
Copy link
Contributor

KiChjang commented Oct 13, 2016

@imjacobclark Is this still being worked on?

@ducks
Copy link

ducks commented Oct 18, 2016

I'd like to help finish this one if it's up for grabs.

It looks like the datetime got moved to the upload_nightly.sh but still uses colons.
Was that the only outstanding change needed besides a rebase?

@aneeshusa
Copy link
Contributor

aneeshusa commented Oct 27, 2016

@rjgoldsborough Yes, please feel free to submit a PR for this!

This needs a rebase, but if you look at python/servo/package_commands.py you'll see we replace colons by dashes, which is also behavior we need to keep.

@ghost
Copy link

ghost commented Nov 4, 2016 •
edited by ghost
Loading

Closing based on inactivity (@rjgoldsborough has started working on it in #14058)

@ghost ghost closed this Nov 4, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@aneeshusa aneeshusa

Labels

S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make etc/ci/upload_nightly.sh responsible for putting datetimes into filenames

7 participants

@imjacobclark @highfive @bors-servo @aneeshusa @KiChjang @ducks @SimonSapin