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

Avoid direct upgrade to v2 #770

Merged
merged 1 commit into from
Aug 8, 2018

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Jul 31, 2018

refs #759

  • extend version resolver

  • detect major version jumps

  • optimised code structure (use proper force option - this was hard to maintain/understand)

  • add unit tests

  • do manual test

@kirrg001
Copy link
Contributor Author

@acburdine This PR requires the implementation of the --v1 flag support.

@vikaspotluri123
Copy link
Member

Would we want to cache the results of the yarn info call? It seems wasteful to get all the versions of Ghost 2x 😛

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 2, 2018

Waiting for a merge of #772. Then i'll finalise and optimise this PR.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 2, 2018

Would we want to cache the results of the yarn info call? It seems wasteful to get all the versions of Ghost 2x 😛

Not sure i understand 🤔 Could you please expand your suggestion? Thanks :)

@vikaspotluri123
Copy link
Member

i.e.

let cache = false;

module.exports = function resolveVersion(...) {

// precheck conditions here

return Promise.resolve(cache || yarn(...)).then(...)

Basically since versionResolver gets called twice (2x) there's no point in making the yarn call twice

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 3, 2018

I see. Yeah we can do that 👍

@kirrg001 kirrg001 force-pushed the avoid-direct-upgrade-to-v2 branch 2 times, most recently from 2e39b2d to 81cf6d8 Compare August 7, 2018 13:48
@kirrg001 kirrg001 changed the title [WIP] Avoid direct upgrade to v2 Avoid direct upgrade to v2 Aug 7, 2018
@coveralls
Copy link

coveralls commented Aug 7, 2018

Coverage Status

Coverage increased (+0.0007%) to 99.829% when pulling fecf402 on kirrg001:avoid-direct-upgrade-to-v2 into 1036c48 on TryGhost:1.9.

@kirrg001 kirrg001 force-pushed the avoid-direct-upgrade-to-v2 branch 2 times, most recently from fa0e5c9 to cee24a8 Compare August 7, 2018 13:58
@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 7, 2018

Basically since versionResolver gets called twice (2x) there's no point in making the yarn call twice

I have changed the code - it was ugly to call the resolver twice. Cache is not needed anymore :)

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 7, 2018

@vikaspotluri123 @acburdine This is ready for review. Would be cool if you could double check if i have missed a case.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 7, 2018

Okay i just discovered one case:

  • You are on v2.
  • You run ghost update --force --v1
  • We should throw an error.
  • Will add

refs TryGhost#759

- extended version resolver
- detect major version jumps
@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 7, 2018

Okay i just discovered one case:

Added

@kirrg001
Copy link
Contributor Author

kirrg001 commented Aug 8, 2018

I'll merge this into the 1.9 branch now. I'll open a new PR tomorrow to see all the changes from 1.9 and i'll add a QA list. If you have time helping with testing, let me know!

@kirrg001 kirrg001 merged commit 2f3de69 into TryGhost:1.9 Aug 8, 2018
acburdine pushed a commit to acburdine/Ghost-CLI that referenced this pull request Aug 16, 2018
refs TryGhost#759
- extended version resolver
- detected major version jumps
- optimised code around the force flag
- added unit tests
acburdine pushed a commit that referenced this pull request Aug 16, 2018
refs #759
- extended version resolver
- detected major version jumps
- optimised code around the force flag
- added unit tests
acburdine pushed a commit that referenced this pull request Aug 16, 2018
refs #759
- extended version resolver
- detected major version jumps
- optimised code around the force flag
- added unit tests
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.

4 participants