CI dogfood improvements #1984

aluzzardi · 2022-04-01T02:24:42Z

DEPENDS ON #1983 and #1971

Improve CI configuration:

Use official Go package
Use golangci-lint package
Fix node_modules exclusion for local source
Improve CUE performance by optimizing cache hits

/cc @samalba @marcosnils

To make this happen, there's also some other universe changes:

universe: go: do not hardcode default platform
universe: go: improve cache management
client filesystem: remove .dagger from default exclusion
universe: go: add golangci-lint support

/cc @helderco @shykes

aluzzardi · 2022-04-01T02:56:09Z

Dogfooding notes/wishlist @shykes @helderco @samalba:

I wish I could do dagger do build lint (a la Makefile)
I wish console outputs were available. It would have been a tremendous help to debug. Also very helpful to inform what just happened (e.g. dagger do build should print the revision we just built). I hacked around by writing a file and then removed that line before committing
I wish the platform defaulted to buildkitd's platform. The performance is horrible in any other setting. I know we had long discussions around the API for this, but it's so important my recommendation would be to default to buildkitd and provide a --platform flag (so we don't need to define the whole CUE DX before making this change)
It would be nice if dagger was GitHub-aware. I split build and lint into two different GH jobs because I want to see them separately in the status. However, that doesn't do any good with caching. Ideally the dagger GHA could report individual actions as "statuses" in the PR?
The docker.#Run / docker.#Build experience was a bit challenging: not sure when to use one or the other. On the one hand, I prefer usingdagger.#Build to define pipelines (e.g. do this, do that, XXX) -- much better than _doA: ... _doB: .... On the other hand, I can't do that as soon as I want to export files. I started writing docker.#Build steps then had to convert to manual operations (see actions.version)
Also, I'm using docker.#Build like a dagger.#Pipeline -- I'm not really building anything
Converting this line from our Makefile resulted in 40 lines of CUE: go build -ldflags '-s -w -X go.dagger.io/dagger/version.Revision=$(git rev-parse --short HEAD). Mostly because of the point above
Language Server: I spent my time jumping between upstream definitions to figure out the interface. I was able to do that because universe happens to be in the same repo as the CI config, anything else would have been a major PITA. Having auto-completion would have been a life saver.

aluzzardi · 2022-04-01T03:11:01Z

On performance:

CI takes 47s to build dagger (make dagger), then dagger do build itself takes 1m19s to rebuild itself. Not sure why it's slower?

Linting is worse. 1m19s vs 5m28s. No idea why either

TomChv

Nice improvement, I added a few comments and questions

.github/workflows/dagger-ci.yml

ci/main.cue

TomChv · 2022-04-01T11:59:47Z

pkg/universe.dagger.io/go/golangci/lint.cue

+)
+
+// Lint using golangci-lint
+#Lint: {


golangci-lint can take a lot of flags to configure the lint. That would be useful to expose go.#Container instead of putting it in container key so the user can add fields to command.flags

We're already exposing go.#Container in container, not sure what you mean?

If someone want to customise flag, he will need to do

go.#Lint & { container: { flags: ... } )

But if go.#Container isn't in container key, he'll just have to do

go.#Lint & { flags: ... )

Yep, agree with you. I followed the same convention as go.#Build though -- should we update that one?

The difference is that go.#Build produce an output that isn't an image but binaries so I didn't wanted to make users think they can use go.#Build as part of docker.#Build.

In you case, it's should be possible so I'm not sure we should update go.#Build ; just updating go.#Lint is enough IMO

pkg/universe.dagger.io/go/container.cue

shykes · 2022-04-01T17:30:17Z

Dogfooding notes/wishlist @shykes @helderco @samalba:

I wish I could do dagger do build lint (a la Makefile)

I wish console outputs were available. It would have been a tremendous help to debug. Also very helpful to inform what just happened (e.g. dagger do build should print the revision we just built). I hacked around by writing a file and then removed that line before committing

I wish the platform defaulted to buildkitd's platform. The performance is horrible in any other setting. I know we had long discussions around the API for this, but it's so important my recommendation would be to default to buildkitd and provide a --platform flag (so we don't need to define the whole CUE DX before making this change)

It would be nice if dagger was GitHub-aware. I split build and lint into two different GH jobs because I want to see them separately in the status. However, that doesn't do any good with caching. Ideally the dagger GHA could report individual actions as "statuses" in the PR?

The docker.#Run / docker.#Build experience was a bit challenging: not sure when to use one or the other. On the one hand, I prefer usingdagger.#Build to define pipelines (e.g. do this, do that, XXX) -- much better than _doA: ... _doB: .... On the other hand, I can't do that as soon as I want to export files. I started writing docker.#Build steps then had to convert to manual operations (see actions.version)

Also, I'm using docker.#Build like a dagger.#Pipeline -- I'm not really building anything

Converting this line from our Makefile resulted in 40 lines of CUE: go build -ldflags '-s -w -X go.dagger.io/dagger/version.Revision=$(git rev-parse --short HEAD). Mostly because of the point above

Language Server: I spent my time jumping between upstream definitions to figure out the interface. I was able to do that because universe happens to be in the same repo as the CI config, anything else would have been a major PITA. Having auto-completion would have been a life saver.

Very useful feedback, could you make it a separate issue so we don't lose it? Thanks!

helderco · 2022-04-01T18:53:52Z

Great stuff 🙂

Dogfooding notes/wishlist @shykes @helderco @samalba:

I wish I could do dagger do build lint (a la Makefile)

Me too! And it makes it more portable. (windows anyone?)

I wish console outputs were available. It would have been a tremendous help to debug. Also very helpful to inform what just happened (e.g. dagger do build should print the revision we just built). I hacked around by writing a file and then removed that line before committing

Yeah, that's also what I've been doing. You know I'm partial to this 🙂

I wish the platform defaulted to buildkitd's platform. The performance is horrible in any other setting. I know we had long discussions around the API for this, but it's so important my recommendation would be to default to buildkitd and provide a --platform flag (so we don't need to define the whole CUE DX before making this change)

Not aware of this. Want to know more.

It would be nice if dagger was GitHub-aware. I split build and lint into two different GH jobs because I want to see them separately in the status. However, that doesn't do any good with caching. Ideally the dagger GHA could report individual actions as "statuses" in the PR?

This has been mentioned by multiple users.

The docker.#Run / docker.#Build experience was a bit challenging: not sure when to use one or the other. On the one hand, I prefer usingdagger.#Build to define pipelines (e.g. do this, do that, XXX) -- much better than _doA: ... _doB: .... On the other hand, I can't do that as soon as I want to export files. I started writing docker.#Build steps then had to convert to manual operations (see actions.version)

Interesting. I've been through this in January a lot, but it was because of the nesting issue. I know how frustrating it is to break those steps into correctly hooked fields. One small typo and you have an error that's not the root cause (incorrect #FS leading to errors elsewhere).

Also, I'm using docker.#Build like a dagger.#Pipeline -- I'm not really building anything

Yes, if there was a generic thing to hook any input: _ it could be reused for other things with the same workflow. I've done this in an abstraction for adding files to an #FS (instead of #Image).

Converting this line from our Makefile resulted in 40 lines of CUE: go build -ldflags '-s -w -X go.dagger.io/dagger/version.Revision=$(git rev-parse --short HEAD). Mostly because of the point above

I'll look into that more closely.

Language Server: I spent my time jumping between upstream definitions to figure out the interface. I was able to do that because universe happens to be in the same repo as the CI config, anything else would have been a major PITA. Having auto-completion would have been a life saver.

Yes, and having full reference docs could help as well.

aluzzardi · 2022-04-01T19:10:50Z

Thanks for the response @helderco :)

I logged this into a separate issue: #1993

EDIT: @helderco mind copy pasting your message into that issue?

I just added spellcheck support and markdownlint -- for now in dagger/ci/pkg so we don't need for those packages to be "universe"quality.

I'd like to migrate our own CI to dagger ASAP so we can feel the pain :)

aluzzardi · 2022-04-01T19:27:38Z

Update:

Moved universe.dagger.io/go/golangci into ci/pkg/golangci so we don't need universe quality
Add pkg/markdownlint, pkg/shellcheck

dagger CI linting is on par with Makefile linting

TomChv

Added a little comment, another question : package that you create specially for a plan should be in cue.mod/pkg or cue.mod/usr?
If I remember well, cue.mod/pkg is only for external dependencies

ci/main.cue

aluzzardi · 2022-04-01T20:25:16Z

Added a little comment, another question : package that you create specially for a plan should be in cue.mod/pkg or cue.mod/usr?

In this case I decided to add my packages to ci/pkg -- not in cue.mod. I think cue.mod is better suited for dependencies, not for my own code.

TomChv · 2022-04-01T20:32:55Z

Added a little comment, another question : package that you create specially for a plan should be in cue.mod/pkg or cue.mod/usr?

In this case I decided to add my packages to ci/pkg -- not in cue.mod. I think cue.mod is better suited for dependencies, not for my own code.

Clear!

shykes

Love the factoring out into sub-packages
Reverting back to client.filesystem.".." is a regression (unnecessary tight coupling to current workdir).

shykes · 2022-04-04T21:43:16Z

.github/workflows/dagger-ci.yml

+      - "**.sh"
+      - "**.bash"
+      - "**.go"
+      - "**.cue"
+      - "**.bats"
+      - "Makefile"
+      - "go.mod"
+      - "go.sum"
+      - ".github/workflows/dagger-ci.yml"


I can't wait to remove those and just rely on Dagger to do its thing...

ci/main.cue

shykes · 2022-04-04T21:45:32Z

ci/main.cue

-				contents: core.#CacheDir & {
-					id: "go mod cache"
-				}
+		_source: client.filesystem["../"].read.contents


Same comment as above: access .. for this is an anti-pattern.

shykes · 2022-04-04T21:47:01Z

ci/main.cue

-				# Check that all formatted files where committed
-				test -z $(git status -s . | grep -e '^ M'  | grep .cue | cut -d ' ' -f3)
-				"""#
+			cue: docker.#Build & {


Maybe add a FIXME to spin this out into a cuelangci package like the equivalent golangci?

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

- Use official Go package - Use golangci-lint package - Fix node_modules exclusion for local source - Improve CUE performance by optimizing cache hits Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

addressed

aluzzardi requested review from helderco, shykes and a team as code owners April 1, 2022 02:24

aluzzardi force-pushed the ci-improvement branch 6 times, most recently from 5e9f1cd to 2333709 Compare April 1, 2022 02:44

aluzzardi force-pushed the ci-improvement branch from f1765ac to 54b77ba Compare April 1, 2022 02:58

aluzzardi force-pushed the ci-improvement branch 3 times, most recently from aa526e7 to fbad661 Compare April 1, 2022 07:41

TomChv requested changes Apr 1, 2022

View reviewed changes

TomChv added kind/enhancement area/ci labels Apr 1, 2022

aluzzardi force-pushed the ci-improvement branch from fbad661 to c9be445 Compare April 1, 2022 18:34

aluzzardi mentioned this pull request Apr 1, 2022

✨ CI: dogfooding feedback #1993

Closed

aluzzardi force-pushed the ci-improvement branch from c9be445 to 1c1eff1 Compare April 1, 2022 18:49

aluzzardi force-pushed the ci-improvement branch from c3fc85b to d51b09f Compare April 1, 2022 19:26

TomChv previously requested changes Apr 1, 2022

View reviewed changes

ci/main.cue Outdated Show resolved Hide resolved

aluzzardi mentioned this pull request Apr 1, 2022

✨ META: dagger dev DX #1992

Closed

samalba mentioned this pull request Apr 4, 2022

Convert our own Makefile to dagger #1522

Closed

shykes requested changes Apr 4, 2022

View reviewed changes

aluzzardi added 9 commits April 4, 2022 15:33

universe: go: do not hardcode default platform

f90b1ad

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

universe: go: improve cache management

3e8c937

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

client filesystem: remove .dagger from default exclusion

0a6fa03

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

task: source: fix vertex name

cf3993a

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

universe: go: add golangci-lint support

b989c5d

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

ci: use dagger from source

8fe29cd

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

ci: improve dogfood configuration

31daeb0

- Use official Go package - Use golangci-lint package - Fix node_modules exclusion for local source - Improve CUE performance by optimizing cache hits Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

ci: lint shell

517262d

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

ci: lint markdown files

50b36a5

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

aluzzardi force-pushed the ci-improvement branch 2 times, most recently from 1fd0f92 to a8ccf33 Compare April 5, 2022 00:50

ci: move ci infra top-level

93d2224

Signed-off-by: Andrea Luzzardi <aluzzardi@gmail.com>

aluzzardi force-pushed the ci-improvement branch from a8ccf33 to 93d2224 Compare April 5, 2022 01:06

shykes approved these changes Apr 5, 2022

View reviewed changes

aluzzardi merged commit a717ec0 into dagger:main Apr 5, 2022

aluzzardi deleted the ci-improvement branch April 5, 2022 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI dogfood improvements #1984

CI dogfood improvements #1984

aluzzardi commented Apr 1, 2022 •

edited

aluzzardi commented Apr 1, 2022 •

edited

aluzzardi commented Apr 1, 2022

TomChv left a comment

TomChv Apr 1, 2022

aluzzardi Apr 1, 2022

TomChv Apr 1, 2022

aluzzardi Apr 1, 2022

TomChv Apr 1, 2022

shykes commented Apr 1, 2022

helderco commented Apr 1, 2022

aluzzardi commented Apr 1, 2022 •

edited

aluzzardi commented Apr 1, 2022

TomChv left a comment

aluzzardi commented Apr 1, 2022

TomChv commented Apr 1, 2022

shykes left a comment

shykes Apr 4, 2022

shykes Apr 4, 2022

shykes Apr 4, 2022

aluzzardi Apr 5, 2022

CI dogfood improvements #1984

CI dogfood improvements #1984

Conversation

aluzzardi commented Apr 1, 2022 • edited

aluzzardi commented Apr 1, 2022 • edited

aluzzardi commented Apr 1, 2022

TomChv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shykes commented Apr 1, 2022

helderco commented Apr 1, 2022

aluzzardi commented Apr 1, 2022 • edited

aluzzardi commented Apr 1, 2022

TomChv left a comment

Choose a reason for hiding this comment

aluzzardi commented Apr 1, 2022

TomChv commented Apr 1, 2022

shykes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi commented Apr 1, 2022 •

edited

aluzzardi commented Apr 1, 2022 •

edited

aluzzardi commented Apr 1, 2022 •

edited