-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Remove travis jobs with current stable BlockBundle for master branches #400
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay now we can discuss on a diff. 😛
Can't we just use
4? Normally it should work with branch alias.The thing that cause issue to me is to just remove sonata_block. We should test at least one version IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new version of AdminBundle is incompatible with current stable BlockBundle and can work only with a new version of BlockBundle, so i think it is not an option - AdminBundle must be tested only with BlockBundle
4.0.A dependency on
sonata-project/block-bundlewas already added to composer.json, so I just removed dependency on^3.11from it and left only^4.0@devin my PR for AdminBundle, see diff https://github.com/sonata-project/SonataAdminBundle/pull/4938/files.DashboardBundle, DoctrinePhpcrAdminBundle, FormatterBundle, PageBundle, SeoBundle and TimelineBundle has direct dependency on BlockBundle too. When i will update these bundles, i will update that dependency too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@covex-nn Indeed, if the deps is already here, it will change nothing for now. But it would be better to keep it with only
4in order to remember to test multiple versions in the future IMHO.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soullivaneuh a line with
sonata_block: ['4']will generate two jobs for builds, see .travis-ci.yml:env: SONATA_BLOCK=4.*env: SONATA_BLOCK='dev-master as 4.x-dev'(this job is allowed to fail)These jobs first try to require another SonataBlockBundle (they take a version from SONATA_BLOCK env var) and then do all other stuff. See before_install_test.sh.twig
BlockBundle v4 was not relesed yet. So, after updating
sonata_block: ['3']tosonata_block: ['4']all builds will always fail because of job withSONATA_BLOCK=4.*.I think, that such jobs should be added only for packages, that really has multiple versions. But there won't be multiple versions for SonataBlockBundle, there will be just
4, because bundles will not be not compatible with lower versions =) Also, i think that all jobsSONATA_BLOCK=3.*andSONATA_CORE=3.*are useless now, because everything was already tested with regular jobsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soullivaneuh is there anything else that I could do here? (this PR is blocker for sonata-project/SonataAdminBundle#4938)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@covex-nn Sorry for the time but I try to understand the need: Any project should work with at least one stable dependency. It's not the case here: https://github.com/sonata-project/SonataAdminBundle/pull/4938/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R29
If we have to release a new major of SonataAdminBundle, what should we do? At least, it should works with both versions or block bundle v4 should be release first.
Also, to test non-released dependencies, we already have the dev-master alias. But we may update the travis twig file to require 3.*@dev each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soullivaneuh thank you for review
I will try to make AdminBundle compatible with BlockBundle
^3.11 || ^4.0@dev. if I succeed, i will close this PR. I will try to useclass_aliasfor this, like here https://github.com/symfony/phpunit-bridge/blob/4.0/SymfonyTestsListener.php