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

Add script to skip CircleCI on guides #2955

Merged

Conversation

jacobherrington
Copy link
Contributor

This is based on the discussion here: Moya/Moya#1286

I'm not super familiar with Circle, but if this doesn't work for some obvious reason feel free to close.

@aitbw
Copy link
Contributor

aitbw commented Nov 15, 2018

🤔 but you can skip CI builds by adding [ci skip] to the commit message, here's more information about it

@jacobherrington
Copy link
Contributor Author

@aitbw Yes, that is correct, but this is automated. Not all who wish to contribute to the docs will know to use [ci skip] or [skip ci] in the commit message.

@jacobherrington
Copy link
Contributor Author

Not to mention that I'd just like to hear opinions on this idea. There may be a very good reason to run CI on doc changes. 🤷‍♂️

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I really like that! Thanks for saving precious resources 🌳

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I think it's a great idea, but I'd prefer to have this into the CircleCi config directly without another script in the root of the project

@@ -16,6 +16,7 @@ jobs:
POSTGRES_USER: root
parallelism: &parallelism 3
steps: &steps
- run: skip-ci.sh && circleci step halt || true # skip Circle for guides
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have the script inline without the need to add another script file? I personally prefer a less readable line than a new file in the project that is used in a single point.

Copy link
Member

Choose a reason for hiding this comment

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

A good place for the script could be bin/circleci-halt-for-doc-only-changes or .circleci/halt-for-doc-only-changes and move everything inside it. That way what it does should be self-evident and well contained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that suggestion @elia do you agree with this @kennyadsl?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, .circleci/bin/halt-for-doc-only-changes.sh would be my preference

@jacobherrington
Copy link
Contributor Author

Unfortunately this isn't a CircleCI feature, though I wish it was... It requires some creative thinking to solve. I think I could probably figure out a way to do this inside of the CircleCI config, but I'm not sure I see the benefit of doing it that way.

I will see if I can find a way to do that, but it might be simpler to stick with this implementation.

Would it help if we created a scripts/ directory to put things like this? build.sh could live there as well.

@jacobherrington
Copy link
Contributor Author

@kennyadsl Done!

@kennyadsl
Copy link
Member

@jacobherrington 👍 can we please rebase?

@kennyadsl
Copy link
Member

@jacobherrington sorry, I meant squash commits 😬

@jacobherrington
Copy link
Contributor Author

Done @kennyadsl 👍

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

great!

I think it would make sense at this point to keep everything inside the script vs. circleci config which would have just:

- run: .circleci/bin/halt-for-doc-only-changes.sh

The name is pretty explicit and it already says it will halt on a condition 👍

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Approved, I'm also fine with what @elia proposed, up to you if you want to change that.

@tvdeyen tvdeyen merged commit 95bf58a into solidusio:master Nov 23, 2018
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