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.

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

🎨 Backup redirects file before overriding #9051

Merged
merged 1 commit into from
Sep 25, 2017

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Sep 25, 2017

refs #9028

  • if you upload a redirects file and a redirects file exists already, we backup this file to data/redirects-YYYY-MM-DD-HH-mm-ss.json
  • decrease chance of random test failures by not comparing date format with seconds

NOTE: We would love to use the promise support by fs-extra which was added in 3.x, but they are using native Promises and Ghost uses Bluebird. There is an open issue which discusses this topic. As long as fs-extra does not support alternative Promise libaries, we have to use promisify for fs extra calls, otherwise e.g. the catch behaviour is different.

refs TryGhost#9028

- if you upload a redirects file and a redirects file exists already, we backup this file to `data/redirects-YYYY-MM-DD-HH-mm-ss.json`
- decrease chance of random test failures by not comparing date format with seconds
@kirrg001
Copy link
Contributor Author

@kevinansfield Added date-time format for redirect backup files as discussed in slack.

@kevinansfield
Copy link
Contributor

👍 local install is creating backups OK. I'm seeing backups created for failed uploads too - is that expected?

@kirrg001
Copy link
Contributor Author

I'm seeing backups created for failed uploads too - is that expected?

This would make the logic more complicated, so i decided to ignore this. In worst case it would create a backup of a none changed redirects file. A failed upload should only occur if your file format is wrong.

@kevinansfield kevinansfield merged commit 22017b8 into TryGhost:master Sep 25, 2017
@kevinansfield kevinansfield deleted the backup-redirectsfile branch September 25, 2017 17:36
@ErisDS
Copy link
Member

ErisDS commented Sep 26, 2017

NOTE: We would love to use the promise support by fs-extra which was added in 3.x, but they are using native Promises and Ghost uses Bluebird. There is an open issue which discusses this topic. As long as fs-extra does not support alternative Promise libaries, we have to use promisify for fs extra calls, otherwise e.g. the catch behaviour is different.

We've run into this before, there is a solution written here with a big explanation.

We already have an overrides file, I think it might be a good idea to add
global.Promise = require('bluebird') to it? cc @kirrg001

@kirrg001
Copy link
Contributor Author

We already have an overrides file, I think it might be a good idea to add
global.Promise = require('bluebird') to it? cc @kirrg001

I considered this and read about it a lot already. There are arguments for and against doing that. See for example this comment.

@ErisDS
Copy link
Member

ErisDS commented Sep 26, 2017

I think for Ghost, where we already have an overrides.js file that lives at all the right places, it makes more sense, than it might in other cases.

For us, I think the problem is the opposite. I've wasted too much time bashing my head against a promise chain because I used a catch predicate on a promise that wasn't a bluebird promise because it came from some library somewhere and wasn't handled & "wrapped" with bluebird. We saw this same problem set us back a couple of days on the image size refactor project.

We are so accustomed to using catch predicates, and they work so well with the way our code is written, I think we would be less likely to trip ourselves if everything was bluebird than we currently are with a mix.

That's just my opinion though 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants