Skip to content

Rethink release workflow in terms of environments#11612

Merged
tgamblin merged 6 commits intospack:developfrom
scottwittenburg:add-gitlab-ci-schema-to-environment
Sep 14, 2019
Merged

Rethink release workflow in terms of environments#11612
tgamblin merged 6 commits intospack:developfrom
scottwittenburg:add-gitlab-ci-schema-to-environment

Conversation

@scottwittenburg
Copy link
Copy Markdown
Contributor

@scottwittenburg scottwittenburg commented Jun 4, 2019

This PR supplants #10771, as it provides much of the same (or similar) functionality in a cleaner way. This PR is based on #11057, as it takes advantage of some of the features added to environments there (definitions, matrix). The goals of this PR, at a high level, are:

  1. Improve the way a release is described and how it's component jobs are mapped to gitlab-ci runners
  2. Implement a basic multi-repo approach to CI (where the first phase of CI is used to generate the jobs, and the second phase actually builds the specs).

Use of Spack environments as release descriptors

In this section, we describe the structure of the new gitlab-ci and cdash sub-sections of the environment schema and how they can be used to describe a release. This PR contains an example spack.yaml file illustrating the newly proposed bits (located in lib/spack/docs/example_files), while a shorter example is shown below for reference:

spack:
  definitions:
  - pkgs:
    - [email protected]
  - compilers:
    - '%[email protected]'
  - oses:
    - os=ubuntu18.04
    - os=centos7
  specs:
  - matrix:
    - [$pkgs]
    - [$compilers]
    - [$oses]
  mirrors:
    cloud_gitlab: https://mirror.spack.io
  gitlab-ci:
    mappings:
      - spack-cloud-ubuntu:
        match:
          - os=ubuntu18.04
        runner-attributes:
          tags:
            - spack-k8s
          image: spack/spack_builder_ubuntu_18.04
      - spack-cloud-centos:
        match:
          - os=centos7
        runner-attributes:
          tags:
            - spack-k8s
          image: spack/spack_builder_centos_7
  cdash:
    build-group: Release Testing
    url: https://cdash.spack.io
    project: Spack
    site: Spack AWS Gitlab Instance

The definitions section, as well as the matrix within the specs section illustrate why this PR is based on #11057, although it is possible that everything in this PR might work fine if specs were explicitly enumerated in the spack.yaml environment file instead.

The gitlab-ci section describes a set of gitlab runners and the conditions under which the specs described in the environment should be assigned to be built by one of the runners. Currently this section has only one sub-section, mappings, and so perhaps the mappings intermediate level could be removed. At any rate, each entry within the list of mappings corresponds to a known gitlab runner, where the match section is used in assigning a release spec to one of the runners, and the runner-attributes section is used to configure the spec/job for that particular runner.

The optional cdash section provides information that will be used by the release-jobs command for reporting to CDash. All the jobs generated from this environment will belong to a "build group" within CDash that can be tracked over time. As the release progresses, this build group may have jobs added or removed. The url, project, and site are used to specify the CDash instance to which build results should be reported.

Assignment of specs to runners

The mappings section corresponds to a list of runners, and during assignment of specs to runners, the list is traversed in order looking for matches, the first runner that matches a release spec is assigned to build it. The match section within each runner mapping section is a list of specs, and if any of those specs match the release spec (the spec.satisfies() method is used), then that runner is considered a match.

Configuration of specs/job for a runner

Once a runner has been chosen to build a release spec, the runner-attributes section provides information determining details of the job in the context of the runner. The runner-attributes section must have a tags key, which is a list containing at least one tag used to select the runner from among the runners known to a gitlab instance. For Docker executor type runners, the image key is used to specify the Docker image used to build the release spec, while other types of runners the variables key will be useful to pass any information on to the runner which it needs to do its work (e.g. scheduler parameters, etc.).

Summary of .gitlab-ci.yml generation algorithm

All specs yielded by the matrix (or all the specs in the environment) have their dependencies computed, and the entire resulting set of specs are staged together before being run through the gitlab-ci/mappings entries, where each staged spec is assigned a runner. Staging is the name we have given to the process of figuring out in what order the specs should be built, taking into consideration gitlab ci rules about jobs/stages. In the staging process the goal is to maximize the number of jobs in any stage of the pipeline, while ensuring that the jobs in any stage only depend on jobs in previous stages (since those jobs are guaranteed to have completed already). As a runner is determined for a job, the information in the runner-attributes is used to populate various parts of the job description that will be used by gitlab ci. Once all the jobs have been assigned a runner, the .gitlab-ci.yml is written to disk.

The short example provided above would result in the readline, ncurses, and pkgconf packages to be staged and built on two different runners. The runner named spack-cloud-centos (the names have no meaning, and can be anything) will be assigned to build all three packages for centos7, while the spack-cloud-ubuntu runner will be assigned to build the same set of packages for ubuntu-18.04. The resulting .gitlab-ci.yml will contain 6 jobs in three stages. Once the jobs have been generated, the presence of a --cdash-credentials argument to the release-jobs script would result in all of the jobs being put in a build group on CDash called Release Testing (that group will be created if it didn't already exist).

Comparing the previous approach to the new one

Previously, a release was described in the release.yaml file, which provided a file-based representation of the CombinatorialSpecSet type. This type allowed to list packages, package versions, compilers, and compiler versions, and would then compute and yield the full cross-product of those lists. Then the spack release-jobs ... command would process the resulting specs, matching them up against entries in the os-container-mapping.yaml file to find docker images which could build the specs, and ultimately generate the .gitlab-ci.yml file.

There were multiple drawbacks to the previous approach. One issue was that the os-container-mapping.yaml assumed that all ci jobs would be run in a gitlab runner configured with the Docker executor type runner, which made it difficult for sites to adopt the release workflow. The need for Docker containers was, itself, another drawback of the approach, but at the time the release workflow was initially developed, specs could not be guaranteed to be concretized properly if the concretization was not performed on a machine matching the osarch of the spec. Thus, in order to generate the .gitlab-ci.yml, we needed first to organize the release into groups of specs that could all be concretized the in the same container, then implement a micro web-service that could farm that concretization work out appropriately. While this PR doesn't specifically fix the need to concretize specs in containers, it assumes the existence of such a fix in order to simplify the code required to generate the .gitlab-ci.yml.

The gitlab-ci schema proposed in this PR is meant to replace the os-container-mapping.yml used previously. For reference, the goal of the os-container-mapping.yml file was to aid in mapping each spec from a set of release specs to a gitlab runner which could build the spec. The gitlab-ci schema, when used within an environment with a set of packages, one or more configured mirrors, and a cdash section is the same. While the gitlab-ci schema doesn't do anything fundamentally new, it does represent a few key improvements over the old mapping file:

  1. Previously the mapping was in a separate file and directory, and the relationship between the release.yaml and the os-container-mapping.yml wasn't clear at all. This change co-locates the spec -> runner mapping with the description of the specs in the release.
  2. Previously there was only a single, fixed mapping which made customization difficult or impossible.
    Now every release can be provided with a custom mapping.
  3. Previously Docker executor gitlab runners were assumed, which made it impossible to target, for example, setuid runners designed by/for sites. This change allows the release writer to specify tags to select the runner for a job, and also allows the author to specify variables to provide to the selected runner.

Multi-repo CI approach

Another thing this PR does is take a first stab at a multi-repo approach to the release workflow. Because gitlab-ci does not yet allow for dynamic generation of jobs, a so-called "pre-ci" phase is needed to generate the .gitlab-ci.yml from the release description. This was previously conceived to be handled by standing up a microservice that could generate the ci file on demand, and hosting it at some known endpoint. The endpoint would allow provision of the commit sha to be checked out when generating the ci jobs (via a url parameter). Then the gitlab-ci include feature would be used to include the dynamically generated jobs. Unfortunately, the include feature does not support variable interpolation, and so the important information of which commit to check out when generating the jobs could not be communicated to the microservice. Hence, the need for two stages of ci.

The multi-repo approach proposed in this PR assumes that the first stage of the release workflow will be handled by a gitlab repo, for example a Spack mirror. This CI on this repo will generate the actual release jobs as a .gitlab-ci.yml, and then push a commit to the gitlab repo where the actual release jobs can be run.

The proposed approach allows for either an environment file within the Spack repo, or else an environment file in another arbitrary repo to be used as the release description. At a high level, the pre-ci phase in this PR (the actual implementation is located in bin/generate-gitlab-ci-yml.sh) does the following:

  1. look for a CI environment variable called SPACK_RELEASE_ENVIRONMENT_REPO, if that variable is found, clone the repo, otherwise, assume the Spack repo contains the release environment file
  2. expect a CI environment variable called SPACK_RELEASE_ENVIRONMENT_PATH, using the location of the repo determined in the previous step as the base path, to find the release environment file
  3. activate the environment given in the spack.yaml file from the previous step
  4. run the spack release-jobs ... command
  5. commit the resulting .gitlab-ci.yml file and push the commit to the repo designated by the CI environment variable DOWNSTREAM_CI_REPO.

See this repo for an example that a site could set up to desribed it's own release. In this kind of setup, the site would probably maintain three repos:

  1. small repo containing the release description (spack.yaml)
  2. Spack mirror (pre-ci phase for .gitlab-ci.yml generation)
  3. A repo with runners configured to build the release specs

Optional compiler bootstrapping

This PR also adds support for bootstrapping compilers on systems which may not already have the desired compilers installed. The idea here is that you can specify a list of things to bootstrap in your definitions, and we will guarantee those will be installed in a phase of the pipeline before your release specs, so that you can rely on those packages being available in the binary mirror when you need them later on in the pipeline. At the moment the only viable use-case for bootstrapping is to install compilers.

Here's an example of what bootstrapping some compilers looks like, taken from the example environment/stack included with this PR (lib/spack/docs/example_files/spack.yaml):

spack:
  definitions:
  - compiler-pkgs:
    - '[email protected] os=centos7'
    - '[email protected] os=centos7'
    - '[email protected] os=ubuntu18.04'
    - '[email protected] os=ubuntu18.04'
  - pkgs:
    - [email protected]
  - compilers:
    - '%[email protected]'
    - '%[email protected]'
    - '%[email protected]'
    - '%[email protected]'
    - '%[email protected]'
  - oses:
    - os=ubuntu18.04
    - os=centos7
  specs:
  - matrix:
    - [$pkgs]
    - [$compilers]
    - [$oses]
    exclude:
      - '%[email protected] os=centos7'
      - '%[email protected] os=ubuntu18.04'
  compilers:
    # Include definitions of compilers which will not exist on the pre-ci machine, where
    # the .gitlab-ci.yml will be generated.  This allows spack to "cross-concretize" specs
    # for other operating systems and architectures.
    ...
  gitlab-ci:
    bootstrap:
      - name: compiler-pkgs
        compiler-agnostic: true
    mappings:
      # mappings similar to the example higher up in this description
      ...

In the example above, we have added a list to the definitions called compiler-pkgs (you can add any number of these), which lists compiler packages we want to be staged ahead of the full matrix of release specs (which consists only of readline in our example). Then within the gitlab-ci section, we have added a bootstrap sections which can contain a list of items, each of which refers to a list in the definitions section. These items can either be a dictionary or a string. If you supply a dictionary, it must have a name key whose value must match one of the lists in definitions and it can have a compiler-agnostic key whose value is a boolean. If you supply a string, then it needs to match one of the lists provided in definitions. You can think of the bootstrap list as an ordered list of pipeline "phases" that will be staged before your actual release specs. While this introduces another layer of bottleneck in the pipeline (all jobs in all stages of one phase must complete before any jobs in the next phase can begin), it also means you are guaranteed your bootstrapped compilers will be available when you need them.

The compiler-agnostic key which can be provided with each item in the bootstrap list tells the release-jobs command that any jobs staged from that particular list should have the compiler removed from the spec, so that any compiler available on the runner where the job is run can be used to build the package.

When including a bootstrapping phase as in the example above, the result is that the bootstrapped compiler packages will be pushed to the binary mirror (and the local artifacts mirror) before the actual release specs are built. In this case, the jobs corresponding to subsequent release specs are configured to install_missing_compilers, so that if spack is asked to install a package with a compiler it doesn't know about, it can be quickly installed from the binary mirror first.

Since bootstrapping compilers is optional, those items can be left out of the environment/stack file, and in that case no bootstrapping will be done (only the specs will be staged for building) and the runners will be expected to already have all needed compilers installed and configured for spack to use.

@scottwittenburg
Copy link
Copy Markdown
Contributor Author

@reviewers: I'd like to take a lot of the PR description here and start working it into the documentation, as we decide this is getting close to the direction we want to go.

@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch from 90e137e to c9e8124 Compare June 4, 2019 15:50
opadron
opadron previously requested changes Jun 4, 2019
Copy link
Copy Markdown
Member

@opadron opadron left a comment

Choose a reason for hiding this comment

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

Looks great! Added a few notes.

.gitlab-ci.yml Outdated
- "./bin/generate-gitlab-ci-yml.sh"
tags:
- "spack-k8s"
image: "scottwittenburg/spack_builder_ubuntu_18.04"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At some point, this should be update to spack/ci:ubuntu-18.04. See #11621.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense. I'll take a look at that some time today and provide some feedback as well. If we can get that merged and the images pushed to Dockerhub, I could fix this and have one less thing to wait around for.

else
echo "Cloning environment repo: ${SPACK_RELEASE_ENVIRONMENT_REPO}" >&2
mkdir /work
cd /work
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fine for our CI images, but will likely be a problem for sites running directly on their systems. Better would be to rely on mktemp -d and adding a trap line to ensure that the temp dir is cleaned up before the script exits.

Of course, you'll need the Python equivalents to the above (tempfile and atexit modules) if/once you move this over to a Python implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fine for our CI images, but will likely be a problem for sites running directly on their systems. Better would be to rely on mktemp -d and adding a trap line to ensure that the temp dir is cleaned up before the script exits.

Good suggestion, thanks. I'll do this on the next pass.

Of course, you'll need the Python equivalents to the above (tempfile and atexit modules) if/once you move this over to a Python implementation.

Until you mentioned it here, I hadn't imagined we'd make this a python implementation. I was considering that for the rebuild-package.sh script instead. In that case, there's a lot of work to do, and it's mostly spack commands, so it makes some sense to me do make the whole thing a spack command. Do you think there's a good argument to be made for doing that here as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not particularly, no. Feel free to ignore my point if that doesn't turn out to be a direction you decide to take this work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is fine for our CI images, but will likely be a problem for sites running directly on their systems. Better would be to rely on mktemp -d and adding a trap line to ensure that the temp dir is cleaned up before the script exits.

@opadron Do you know what signals I should provide to trap to make sure the cleanup always happens, no matter how the script exits?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

trap 'rm -rf "$tempdir"' INT TERM QUIT EXIT


cd $env_dir

token_file=/tmp/cdash_auth_token
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Put this in your generated temp dir to avoid name collisions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, good suggestion, will do.

echo "git commit"
git commit -m "$commit_msg"
echo "git commit-tree/reset"
git reset "$( git commit-tree HEAD^{tree} -m ${commit_msg} )"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might want to add a comment explaining what's going on with this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At one point @tgamblin also suggested just pushing the single new commit as well. Perhaps it's worth having a slightly more involved discussion about how we want to handle this?

Otherwise, yes, a comment will be nice here 😄 will do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It really boils down to whether the downstream repo is prepopulated with whatever is the main branch upstream. In this case, sending history should be (much) faster than sending the working copy, since most of the WC is already there. However, to maintain this benefit long term requires that the main downstream branch be continuously synchronized with the main upstream branch. Otherwise (given enough history), sending history becomes much slower.

So, the real trade off made here is that we are always sending what amounts to a tarball of the WC, but in exchange we avoid the need for any external processes to keep the main branches synchronized across the two repos. If there's an easy way to setup this synchronization (that doesn't require a paid GL license), then I'd be all for pushing the commits directly.

Maybe we could add that among the final "housekeeping" tasks we generate in CI?

git status
echo "git push"
git push --force "$DOWNSTREAM_CI_REPO" \
"___multi_ci___:$current_branch"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Might want to add a prefix to the pushed branch name, maybe something like multi-ci-$current_branch. Should make these generated branches easier to identify.

  2. Not sure if you've already thought of this, but at some point at the end of the generated workflow, we're going to want to come back and clean out these auto-generated branches.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Might want to add a prefix to the pushed branch name, maybe something like multi-ci-$current_branch. Should make these generated branches easier to identify.

Yeah, I think that could make sense. Are these branches going to be publicly visible ultimately? If so, it would be nice for people to recognize their branch names, in which case maybe I should tack something onto the end of the name rather than the beginning. If not, it may not matter much. But I agree that having something on there that helps us identify the branches created/pushed by the workflow makes sense.

  1. Not sure if you've already thought of this, but at some point at the end of the generated workflow, we're going to want to come back and clean out these auto-generated branches.

It occurred to me at some point, but I haven't done anything about it yet. Perhaps we could talk about it if/when we have the discussion I mentioned above. I've been thinking it could be nice to have a history of the generated .gitlab-ci.yml files maintained somehow. But I'm not married to the idea in any way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regarding point 1: My thinking falls on two points: First, pushing a branch with nothing but a hash for the name does nothing to communicate the branch's purpose. I don't expect human eyes to be looking at these branches regularly, but neither should someone poking around in these internals be scratching their heads wondering to themselves what these branches are all about?

Second, while we expect that in most use cases the downstream repo would exist exclusively for triggering the next phase of CI, I don't think we should ignore the possibility that someone would use a downstream repo that has other purposes. In this case, we would certainly expect more human eyes to be on these generated branches, but more importantly, I'd want to take every measure to avoid name collisions. Let's not assume we're the only automated process that is generating branches with hashes in their names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding point 1: My thinking falls on two points: First, pushing a branch with nothing but a hash for the name does nothing to communicate the branch's purpose. I don't expect human eyes to be looking at these branches regularly, but neither should someone poking around in these internals be scratching their heads wondering to themselves what these branches are all about?

Just to be clear, we're not pushing a branch with nothing but a hash for the name, right? We're pushing a branch with whatever name the developer gave their own branch. Now, maybe that's not the best idea, because it's not really their branch, but their branch with the appropriate .gitlab-ci.yml added.

Second, while we expect that in most use cases the downstream repo would exist exclusively for triggering the next phase of CI, I don't think we should ignore the possibility that someone would use a downstream repo that has other purposes. In this case, we would certainly expect more human eyes to be on these generated branches, but more importantly, I'd want to take every measure to avoid name collisions. Let's not assume we're the only automated process that is generating branches with hashes in their names.

Agreed, I'll add the prefix you suggested.

# a part of the CI workflow
tags:
- spack-k8s
image: scottwittenburg/spack_builder_ubuntu_18.04
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update image (#11621).

tags:
- spack-k8s
- some-other-tag
image: scottwittenburg/spack_builder_centos_7
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update image (#11621).

@scottwittenburg
Copy link
Copy Markdown
Contributor Author

Thanks for your review @opadron! I owe you the same on #11621 now 😉 and I'll try to get to that today.

@scottwittenburg
Copy link
Copy Markdown
Contributor Author

ping @aashish24

@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch 15 times, most recently from 6fe4682 to c48b779 Compare June 12, 2019 18:10
if not compiler_job:
job_dependencies.append(root_spec.compiler, spec_arch, build_group)

job_variables = {
Copy link
Copy Markdown
Contributor

@paulbry paulbry Jun 19, 2019

Choose a reason for hiding this comment

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

Minor request, what are thoughts on adding something like PIPELINE_ (or SPACK_RELEASE since it's already used?) as a prefix to all variables created as part of this process. Similar to how GitLab CI starts the majority of the variables it includes in a job with CI_.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @paulbry I've been thinking something like that would make sense in order to distinguish clearly between the variables we generate automatically and the ones you may put in your environment yaml (in the runner-attributes section).

@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch 2 times, most recently from a68dca1 to 1233f63 Compare June 19, 2019 17:41
ci_mappings = yaml_root['gitlab-ci']['mappings']

mirror_url = args.mirror_url
ci_cdash = yaml_root['cdash']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will currently lead to ==> Error: 'cdash' if the cdash: object is not defined in the environment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Glad you're trying it out @paulbry, thanks. I'm aware of this issue and it's fixed in my current working branch, which I'll bring over to this PR sometime today. The new work is mostly about bootstrapping compilers, which is optionaly and you should be able to ignore.

@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch from e34bd5f to f89afb8 Compare July 24, 2019 21:17
@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch from 59d5a0a to 3a77378 Compare August 6, 2019 20:50
@tgamblin tgamblin added environments gitlab Issues related to gitlab integration stacks labels Aug 8, 2019
@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch 2 times, most recently from d331a07 to 458c16f Compare August 12, 2019 16:45
@tgamblin tgamblin self-assigned this Aug 16, 2019
@scottwittenburg scottwittenburg force-pushed the add-gitlab-ci-schema-to-environment branch from 458c16f to 71254ea Compare September 3, 2019 23:37
@tgamblin tgamblin force-pushed the add-gitlab-ci-schema-to-environment branch from 71254ea to 79bb20c Compare September 13, 2019 16:46
@scottwittenburg
Copy link
Copy Markdown
Contributor Author

@tgamblin I hadn't noticed, did it need to be rebased?

@tgamblin
Copy link
Copy Markdown
Member

I caused a conflict by flake8-cleaning schema/spec_set.py

Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

@scottwittenburg: this LGTM, and I think we can merge as-is, as I think it's more important to get these features moving with the mainline than it is to ensure that they are perfect. This is not on the critical path for most users.

I don't have any functional concerns with this at the moment, though I have some high level maintenance concerns about the code that should be addressed in follow-up PRs:

  1. We need to get rid of all (or nearly all) of the shell code. Spack's a python project, and Python is easier to maintain for a lot of this logic. So I'd like to see the shell code moved to neatly abstracted commands (or sets of commands) and core Spack classes/components if needed. Right now the CI pipeline code is hard to reuse across different parts of the project, and probably more complex than it needs to be because it is in bash.

  2. We should move the shell scripts in bin to share/spack/qa, or some other non-user-facing directory. The only user facing command in the end should be spack, and we should have a spack pipeline command or something to house the CI logic.

All of this is really for a follow-on PRs -- like I said, the functionality here is awesome and we just need to get it out of bash and into the core of Spack.

@tgamblin tgamblin dismissed citibeth’s stale review September 13, 2019 16:56

This was addressed in the Spack Stacks PR (#11057)

@tgamblin
Copy link
Copy Markdown
Member

@opadron: do you have outstanding concerns?

@opadron
Copy link
Copy Markdown
Member

opadron commented Sep 13, 2019

No, I think we should go ahead and merge!

For the move away from bash, I thought we might want to split up large pieces of the functionality and expose them as commands under a ci subcommand:

spack ci concretize
spack ci update-build-groups
spack ci build $SPEC

etc.

- improve mirror git repo management
- minio s3 implementation needs endpoint_url
- Remove image key from rebuild-index job
- Remove image, rely on tags instead
@tgamblin tgamblin force-pushed the add-gitlab-ci-schema-to-environment branch from 79bb20c to 14e9a97 Compare September 13, 2019 17:18
@tgamblin tgamblin dismissed opadron’s stale review September 13, 2019 19:29

Omar approves per his comment above.

@tgamblin tgamblin changed the title WIP: Rethink release workflow in terms of environments Rethink release workflow in terms of environments Sep 13, 2019
@tgamblin
Copy link
Copy Markdown
Member

@scottwittenburg @opadron: the other thing I did not mention above is that this stuff needs tests. This is yet another reason that it should be moved to Python.

@tgamblin tgamblin merged commit 1050fa5 into spack:develop Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

environments gitlab Issues related to gitlab integration stacks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants