Skip to content

Add current validation performed by the server #36

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

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Apr 27, 2017

No description provided.

README.rst Outdated
* The versions are valid;
* Version defined in the directory must be the oldest amongst those defined in `version_aliases`;
* The package must exist on Packagist;
* The package must have at least one version on Packagist;
Copy link
Member

Choose a reason for hiding this comment

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

do you mean 1 stable version, 1 released version (i.e. counting alpha, beta and RC versions) or are you also counting dev branches ?
If dev versions are counted, this is almost impossible to fail this validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but as I'm making the check anyway, I documented here.

README.rst Outdated
* JSON files must be valid;
* Aliases are only supported in the main repository, not the contrib one;
* Aliases must not be already defined by another package;
* The versions are valid;
Copy link
Member

@javiereguiluz javiereguiluz Apr 27, 2017

Choose a reason for hiding this comment

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

"The versions are valid" -> "All versions listed in the manifest exist as package versions on Packagist" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just that they are valid, updating it

@javiereguiluz
Copy link
Member

javiereguiluz commented Apr 27, 2017

Proposal for another check: the GitHub repo has "php", "symfony", "bundle" and "symfony-bundle" topics, as officially recommended (https://symfony.com/blog/standardizing-the-github-topics-for-symfony-repositories).

@javiereguiluz
Copy link
Member

Another proposal: XLIFF files must be valid (it's easy to make mistakes and break everything ... I did that recently on the Symfony Demo app: symfony/demo#549)

@javiereguiluz
Copy link
Member

Another proposal: service XML files must be valid (we have an XSD for them after all).

@stof
Copy link
Member

stof commented Apr 27, 2017

I don't think adding XLIFF files in the recipe makes much sense, so I'm not sure this makes sense to validate them.

And if you talk about validating the content of the bundle itself, this would be wrong: it would not forbid you to break your own bundle (as there is nothing triggering a recipe PR in this case), but then your own broken bundle would forbid other people to submit their own recipes (as the recipe of your own bundle, which is already merged, would then become invalid for rules validating content in another repo).

@javiereguiluz
Copy link
Member

Another proposal: bundles must be compatible with Symfony.

Not simple but the idea is: Symfony Flex server creates an empty Symfony project and then installs the bundle. The check is to run "bin/console --version" and verify that no errors happen. This check is not perfect, but it rules out a lot of incompatibilities.

@javiereguiluz
Copy link
Member

@stof yes, I was suggesting to check some of the bundle contents. Using Flex to install a bundle and find that your app is broken is not a good experience (of course it's not caused by Flex ... but if we can use Flex to detect those issues...)

@fabpot
Copy link
Member Author

fabpot commented Apr 27, 2017

@javiereguiluz I think those kind of checks are interesting but should be part of a checker that people can install on their bundle repository directly.

@fabpot fabpot merged commit ecc8eb5 into master Apr 27, 2017
fabpot added a commit that referenced this pull request Apr 27, 2017
This PR was merged into the master branch.

Discussion
----------

Add current validation performed by the server

Commits
-------

ecc8eb5 added current validation performed by the server
@Pierstoval
Copy link
Contributor

@javiereguiluz I think those kind of checks are interesting but should be part of a checker that people can install on their bundle repository directly.

Nothing prevents symfony contributors to create such checker and then integrate it into Flex validation server 😉

@fabpot fabpot deleted the validation branch April 28, 2017 23:06
@stof
Copy link
Member

stof commented Apr 29, 2017

@Pierstoval it should not be in the flex validation server, as it would have to run when pushing in the bundle repo, not in the flex recipes repo

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.

6 participants