Skip to content

feat: Add additional parameters to groups in composition files#1338

Merged
laurentsenta merged 14 commits intomasterfrom
feat/builder-config-in-composition
Jun 1, 2022
Merged

feat: Add additional parameters to groups in composition files#1338
laurentsenta merged 14 commits intomasterfrom
feat/builder-config-in-composition

Conversation

@laurentsenta
Copy link
Copy Markdown
Contributor

@laurentsenta laurentsenta commented May 17, 2022 •
edited
Loading

Fixes #1337

This PR allows build_config override in groups (in composition files). See the example below which has a group with a custom docker build image.

It also introduces a modfile option for the docker:go builder that lets you override the go.mod (and go.sum) used to build the application, similar to the -modfile option in go build tools.

[metadata]
  name = "issue-1337-override-builder-configuration"

[global]
  plan = "integrations"
  case = "issue-1337-override-builder-configuration"
  total_instances = 2
  builder = "docker:go"
  runner = "local:docker"

[[groups]]
  id = "v14"
  instances = { count = 1 }

  # --- NEW ---
  [groups.build_config]
    docker_build_image  = "golang:1.14-buster"
    modfile = 'go.v14.mod'
  # --- END NEW ---

  [groups.build]
    selectors = ["v14"]

  [groups.run.test_params]
    expected_version = "v14"

Todo

  • create a corresponding integration test with a similar manifest,
  • add a field build configuration to a composition's group (same content as the global group),
  • make sure the overrides manifest > composition.global > composition.groups are working as expected,
  • make sure the build caching & deduping are taking into account these parameters,
  • find the "right" way to update the BuildKey key,
  • have the test passing,
  • test with libp2p interop.

@laurentsenta laurentsenta marked this pull request as draft May 17, 2022 14:17
@laurentsenta laurentsenta force-pushed the feat/builder-config-in-composition branch 11 times, most recently from 85f705b to 6694035 Compare May 20, 2022 11:06
@laurentsenta laurentsenta changed the title feat: (WIP) Add additional parameters to groups in composition files feat: Add additional parameters to groups in composition files May 20, 2022
@laurentsenta laurentsenta force-pushed the feat/builder-config-in-composition branch 3 times, most recently from 2017ec4 to 2d4e5bb Compare May 20, 2022 15:43
@laurentsenta laurentsenta requested review from brdji and nonsense May 20, 2022 16:20
@laurentsenta laurentsenta marked this pull request as ready for review May 20, 2022 16:20
@laurentsenta laurentsenta force-pushed the feat/builder-config-in-composition branch from 2d4e5bb to f1d694f Compare May 20, 2022 16:26
@laurentsenta laurentsenta requested a review from galargh May 20, 2022 16:26
brdji
brdji reviewed May 23, 2022
View reviewed changes
pkg/build/docker_go.go Outdated
RUN cp /tmp/go.mod /tmp/go.sum ${PLAN_DIR}/

# Copy again the modfiles in case there where overwritten.
COPY /plan/${MODFILE} ${PLAN_DIR}/go.mod
Copy link
Copy Markdown
Contributor

@brdji brdji May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are lines 629 - 632 necessary? Since lines 635-636 will overwrite the go.mod and go.sum, I don't see the point in copying the files to /tmp/, and then back again.

Copy link
Copy Markdown
Contributor

@galargh galargh May 23, 2022

Choose a reason for hiding this comment

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

FYI, GitHub supports multiline comments so in cases like this it's best to comment on all the lines that you mention or link to the ones that are outside the preview - ie. https://docs.google.com/document/d/1mVXsDw74rjFPTu-SAbTOWd0F-5VoIZbHStxJNwWC3ns/edit# in this case.

Good question though - I'm a bit confused about it too. I'll let @laurentsenta answer the original question.

Copy link
Copy Markdown
Contributor Author

@laurentsenta laurentsenta May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch, it's fixed, thanks for the note!

It looks like this now:
https://github.com/testground/testground/pull/1338/files#diff-9b5fa505e6278f70a7479cc5cc065c37fd0c374023b8b89b124d948ffc1025e7R630-R632

We do keep a backup of the modfile, instead of overwriting it.
Because earlier in the Dockerfile, when we go mod download, the command might have updated the go.mod file with the effective module versions.

Copy link
Copy Markdown
Contributor

@brdji brdji May 31, 2022

Choose a reason for hiding this comment

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

I see, the /tmp backup makes sense then!

@brdji brdji mentioned this pull request May 30, 2022
Closed
@laurentsenta
supervisor: trickle down groups build config & use new build key
e3f5d9d
@laurentsenta
test: simplify test to expose missing dockerfile extension
7916fbc
@laurentsenta
feat: rename & fix test plans
7c1af35 
This one might needs some explanation: the go tool
assumes that every sub-directory is part of the project,
except the folders that contain a `go.mod` file or have a
name that starts with `_` (or `.` or `testdata`).

In the case of the integrations tests, we DON'T want to
use a `go.mod` because the point of the test is to
verify that GOMOD rewrites are working.

This is why the "best" solution here is to prefix integration with
a `_`.
@laurentsenta
fix: simplify & comment Group.BuildKey
1ca190b
@laurentsenta
feat: introduce modfile option
13309a4
@laurentsenta
fix: prevent combining modfile and fresh_go_mod options
3d0efad
@laurentsenta
fix: integration test names and runner script
0673d3d
@laurentsenta
docker:go: remove second modfile copy
7a9a81b
@laurentsenta laurentsenta force-pushed the feat/builder-config-in-composition branch from f1d694f to 7a9a81b Compare May 30, 2022 12:13
@laurentsenta laurentsenta merged commit f58abea into master Jun 1, 2022
@laurentsenta laurentsenta deleted the feat/builder-config-in-composition branch June 1, 2022 08:19
@laurentsenta laurentsenta mentioned this pull request Dec 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nonsense nonsense nonsense approved these changes

@galargh galargh Awaiting requested review from galargh

+1 more reviewer

@brdji brdji brdji approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add additional parameters to groups in composition files

4 participants

@laurentsenta @nonsense @galargh @brdji