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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Also validate if lock file is up to date #4219

Merged
merged 1 commit into from Jul 12, 2015

Conversation

localheinz
Copy link
Contributor

This PR

  • adds validation of whether composer.lock is up to date (if it exists), emits an error and exits with non-zero if it's not
  • adds a --no-lock option which will emit a warning instead
  • updates the docs

馃拋 Useful if you want to break the build because someone modified composer.json without updating composer.lock.

Example

Fiddle with composer.json in this repository, for example, change

{
    "autoload": {
        "psr-0": { "Composer": "src/" }
    }
}

to

{
    "autoload": {
        "psr-0": { "Composer": "src" }
    }
}

Before

$ ./bin/composer validate
./composer.json is valid

$ echo $?
0

After

$  ./bin/composer validate
./composer.json is valid
The lock file is not up to date with the latest changes in composer.json.

$ echo $?
1

After (with --no-lock option)

$  ./bin/composer validate
./composer.json is valid
The lock file is not up to date with the latest changes in composer.json.

$ echo $?
0

@localheinz
Copy link
Contributor Author

@Seldaek

Not sure, would you like me to provide tests for this one?

@alcohol
Copy link
Member

alcohol commented Jul 7, 2015

I'm not sure if it makes more sense to move this to a command by itself, or keep it as an option/flag just for install. What if you want your build process to check first, and do a composer update --lock if it isn't up to date (because, reasons)? A command to check/validate the lockfile could also simply be relied on by exit code, and possibly provide some feedback if it is not in sync.

@localheinz
Copy link
Contributor Author

@alcohol

I'm totally open for suggestions! Do you have anything in mind that would make sense?

Maybe a VerifyCommand, resulting in

$ composer verify

or (to leave space for other things which would be useful to verify)

$ composer verify --lock

@alcohol
Copy link
Member

alcohol commented Jul 7, 2015

The first example is too ambiguous, verify doesn't really describe what it does. I would sooner go for something like verify-lock then.

verify --lock looks ok too. Maybe you could also add a --json while you are at it then? It's pretty easy to validate the json schema. See #4217 for some inspiration.

And in the latter scenario, verify would just run all available checks.

@hkdobrev
Copy link
Contributor

hkdobrev commented Jul 7, 2015

It's pretty easy to validate the json schema. See #4217 for some inspiration.

There's the composer validate command which validates composer.json against the JSON config schema.

@alcohol
Copy link
Member

alcohol commented Jul 7, 2015

Ah right I forgot about that. Maybe add the --lock flag to that command?

@localheinz
Copy link
Contributor Author

@alcohol @hkdobrev

Sounds great! Just give me a moment, I'm on the road, traveling.

@localheinz localheinz force-pushed the feature/strict-mode branch 4 times, most recently from b6ca8f2 to 2f2849d Compare July 8, 2015 00:20
@localheinz
Copy link
Contributor Author

@alcohol @hkdobrev

Do you want to take a look?

@localheinz localheinz changed the title Enhancement: Provide a strict option for the Installer Enhancement: Provide a --lock option for the ValidateCommand Jul 8, 2015
@alcohol
Copy link
Member

alcohol commented Jul 8, 2015

Looks good, but I have one concern that maybe @Seldaek might be able to offer some feedback on.

Should the lock file be checked by default as well? Then the option would become --no-check-lock (to skip it instead) which would be more in line with the other options.

@hkdobrev
Copy link
Contributor

hkdobrev commented Jul 8, 2015

馃憤 I also support @alcohol's idea about --no-check-lock.

You should also update CLI docs and CLI command descriptions.

@localheinz
Copy link
Contributor Author

@alcohol @hkdobrev

This is the route I was going down as well, but then figured it would break people's builds if they were using it already, what do you think?

@hkdobrev
Copy link
Contributor

hkdobrev commented Jul 8, 2015

it would break people's builds if they were using it already

I think this is a good thing in this case. And Composer is still in alpha so going for consistency rather than BC is better. Also I think this is rarely used at the moment.

@@ -340,6 +340,7 @@ php composer.phar validate
### Options

* **--no-check-all:** Whether or not Composer does a complete validation.
* **--no-lock:** Do not check if lock file is up to date.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -340,6 +340,7 @@ php composer.phar validate
### Options

* **--no-check-all:** Whether or not Composer does a complete validation.
* **--no-check-lock:** Do not check if lock file is up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@localheinz localheinz changed the title Enhancement: Provide a --lock option for the ValidateCommand Enhancement: Also validate if lock file is up to date Jul 8, 2015
@alcohol
Copy link
Member

alcohol commented Jul 9, 2015

@localheinz: You need to update it, @naderman introduced a merge conflict.

@localheinz localheinz force-pushed the feature/strict-mode branch 2 times, most recently from 6e6f015 to 08834e9 Compare July 9, 2015 13:52
@localheinz
Copy link
Contributor Author

@alcohol

Done!

@@ -37,6 +37,7 @@ protected function configure()
->setDescription('Validates a composer.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

The composer validate command no longer validates composer.json only.

@localheinz
Copy link
Contributor Author

@hkdobrev

Updated!

@alcohol @hkdobrev

What do you think, should this also work when specifying the file argument, that is, attempt to validate a lock file in the same directory as where the specified file has been found?

@alcohol
Copy link
Member

alcohol commented Jul 10, 2015

Yeah that probably makes sense. If they just want to validate a json schema they can skip the lock check. Otherwise, check for a lock file in the same directory.

@localheinz
Copy link
Contributor Author

@alcohol

Amended the PR, please have a look!

@alcohol
Copy link
Member

alcohol commented Jul 10, 2015

Looks good to me.

/cc @Seldaek do you feel this might be too much of a BC change or acceptable?

@Seldaek
Copy link
Member

Seldaek commented Jul 12, 2015

Looks good to me, thanks! I doubt it will create issues but perhaps more people use this in build systems than I imagine.

Seldaek added a commit that referenced this pull request Jul 12, 2015
Enhancement: Also validate if lock file is up to date
@Seldaek Seldaek merged commit 42bfe9c into composer:master Jul 12, 2015
@localheinz
Copy link
Contributor Author

@alcohol @hkdobrev @Seldaek

Thank you for reviewing and merging, I appreciate it!

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.

None yet

4 participants