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

Remove travis jobs with current stable BlockBundle for master branches #400

Closed
wants to merge 1 commit into from

Conversation

covex-nn
Copy link

After removing symfony/templating dependency from all bundles in master branch (see #390), packages won't be compatible with current stable version of sonata-project/block-bundle. Right now travis job with env: SONATA_BLOCK=3.* failing sonata-project/SonataAdminBundle#4938 because of that incompatibility.

This PR removes jobs with env: SONATA_BLOCK=3.* and env: SONATA_BLOCK='dev-master as 3.x-dev' from .travis-ci.yml

@jordisala1991
Copy link
Member

This should be only necessary for master branches right?

@covex-nn
Copy link
Author

Yes, these changes are made for master branches of AdminBundle, DashboardBundle, DoctrinePhpcrAdminBundle, FormatterBundle, PageBundle, SeoBundle and TimelineBundle.

I guess there will be a similar failures for SONATA_ADMIN=3.x too (if sonata-project/SonataAdminBundle#4938 will be accepted), but i am not sure where it should be commented right now.

kunicmarko20
kunicmarko20 previously approved these changes Apr 26, 2018
@@ -5,7 +5,7 @@ admin-bundle:
versions:
symfony: ['3.4']
sonata_core: ['3']
sonata_block: ['3']
#sonata_block: ['3']
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I won't merge commented code/config.

If this one is not compatible with block-bundle 3, why not putting v4/dev-master?

Copy link
Author

@covex-nn covex-nn Apr 26, 2018

Choose a reason for hiding this comment

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

When sonata_block: ['3'] in present in config/projects.yml for some bundle, these lines are added to .travis-ci.yml:

matrix:
  include:
    - php: '7.2'
      env: SONATA_BLOCK=3.*
    - php: '7.2'
      env: SONATA_BLOCK='dev-master as 3.x-dev'
  allow_failures:
    - env: SONATA_BLOCK='dev-master as 3.x-dev'

I cannot put v4/dev-master because after sonata-project/SonataAdminBundle#4938 master branch of AdminBundle will become incompatible with current stable BlockBundle. And travis job for SONATA_BLOCK=3.* will always fail for master branch.

Also, all bundles, that uses BlockBundle are already use 3.x version, so i guess there is no sence in additional job with SONATA_BLOCK=3.*

Also current stable branches of all bundles are already incompatible with master branch of BlockBundle and job with SONATA_BLOCK='dev-master as 3.x-dev' is always failing (but it is allowed).

So, with my PR i tried to remove jobs with SONATA_BLOCK env var for master

@covex-nn covex-nn changed the title Disable travis jobs with current stable BlockBundle for master branches Allow failures for travis jobs with env SONATA_BLOCK Apr 28, 2018
@covex-nn
Copy link
Author

@soullivaneuh i reverted changes. Now i suggest to add jobs with SONATA_BLOCK env var to matrix.allow_failures block of .travis-ci.yml.

With my changes even if master branch of current project is not incompatible with 3.x branch of BlockBundle, all travis build will not fail because of that.

@@ -70,6 +70,11 @@ matrix:
- php: nightly
- env: SYMFONY_DEPRECATIONS_HELPER=0
{% for package_name,package_versions in versions %}
{% if package_name|lower == 'sonata_block' %}
Copy link
Member

Choose a reason for hiding this comment

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

The projects.yml file should be a simple configuration file.

I'm really uncomfortable with this hack, can't we just adjust the configuration file (without comment but remove if justified)?

@covex-nn covex-nn changed the title Allow failures for travis jobs with env SONATA_BLOCK Remove travis jobs with current stable BlockBundle for master branches May 5, 2018
@covex-nn
Copy link
Author

covex-nn commented May 5, 2018

@soullivaneuh i removed all lines with sonata_block: ['3'] for master branches from projects.yml

@@ -5,7 +5,6 @@ admin-bundle:
versions:
symfony: ['3.4']
sonata_core: ['3']
sonata_block: ['3']
Copy link
Member

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.

Copy link
Author

@covex-nn covex-nn May 30, 2018

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-bundle was already added to composer.json, so I just removed dependency on ^3.11 from it and left only ^4.0@dev in 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

Copy link
Member

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 4 in order to remember to test multiple versions in the future IMHO.

What do you think?

Copy link
Author

@covex-nn covex-nn May 30, 2018

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:

  1. with env: SONATA_BLOCK=4.*
  2. with 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'] to sonata_block: ['4'] all builds will always fail because of job with SONATA_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 jobs SONATA_BLOCK=3.* and SONATA_CORE=3.* are useless now, because everything was already tested with regular jobs

Copy link
Author

@covex-nn covex-nn Jul 18, 2018

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)

Copy link
Member

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.

Copy link
Author

@covex-nn covex-nn Jul 20, 2018

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

Any project should work with at least one stable dependency

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 use class_alias for this, like here https://github.com/symfony/phpunit-bridge/blob/4.0/SymfonyTestsListener.php

@core23
Copy link
Member

core23 commented Mar 9, 2019

I think we can close this as @covex-nn is not that active anymore 😢

@core23
Copy link
Member

core23 commented Feb 6, 2020

This is already fixed

@core23 core23 closed this Feb 6, 2020
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.

5 participants