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 support for GitLab CI ENV variable CI_NODE_INDEX starting from 1 #83

Merged
merged 2 commits into from Nov 15, 2018

Conversation

brodock
Copy link
Contributor

@brodock brodock commented Nov 14, 2018

Since GitLab 11.5, you can define parallel: x (2 to 50) to a job and have it
run in multiple workers. You no longer need to define all of them
nor define any environmental variable manually.

You can read more from the documentation here:
https://docs.gitlab.com/ee/ci/yaml/#parallel

You can read about pre-defined ENV variables in GitLab CI here:
https://docs.gitlab.com/ee/ci/variables/README.html#predefined-variables-environment-variables

Since GitLab 11.5, you can define `parallel: x` (2 to 50) to a job and have it
run in multiple workers. You no longer need to define all of them
nor define any environmental variable manually.

You can read more from the documentation here:
https://docs.gitlab.com/ee/ci/yaml/#parallel

You can read about pre-defined ENV variables in GitLab CI here:
https://docs.gitlab.com/ee/ci/variables/README.html#predefined-variables-environment-variables
@brodock
Copy link
Contributor Author

brodock commented Nov 14, 2018

@ArturT I've added support to the new parallel: 10 directive in GitLab CI.

Change include tests and I've added an entry to the CHANGELOG.

Could you please review?

@ArturT
Copy link
Member

ArturT commented Nov 14, 2018

@brodock It looks good. I'm going to merge it and release a new version of knapsack probably tomorrow. Thanks for PR!

@ArturT ArturT merged commit 75b5528 into KnapsackPro:master Nov 15, 2018
@ArturT
Copy link
Member

ArturT commented Nov 15, 2018

@brodock I've just released a new version of the knapsack. Thanks for the help!

@@ -43,6 +43,12 @@ def semaphore_current_thread
def snap_ci_worker_index
index_starting_from_one(ENV['SNAP_WORKER_INDEX'])
end

def gitlab_ci_node_index
return unless ENV['GITLAB_CI']
Copy link

Choose a reason for hiding this comment

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

Should it check CI_SERVER_VERSION as pre 11.5 it was not supported? I guess it's not backward compatible.

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'm not sure if this is the best idea. Before 11.5 the CI would not set CI_NODE_INDEX nor CI_NODE_TOTAL, so anyone using knapsack would have to define a custom variable itself (as we were doing).

There are then few scenarios:

  1. GitLab instance is updated: Because it requires a change in the .gitlab-ci.yaml file, this has no effect on your current jobs, so nothing changes for you.
  2. Knapsack is also updated: When you upgrade the gem, this will break one of your builds because of the index start number change, so you can re-define the CI_NODE_INDEX manually or switch to use the parallel: keyword. Checking for the CI_SERVER_VERSION would not prevent this to break, and as you are updating part of your code, this would blow on your feature branch and you can fix it as normal procedure.
  3. Only Knapsack is updated: It's similar to the case before, the only difference is that checking for the version would make your tests work, but that is hiding something that will eventually break in the near future, but will be harder to debug because you will not have the clue about the knapsack upgrade as cause/consequence.

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

3 participants