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

feat: remove go aliases #7831

Merged
merged 11 commits into from
Jul 10, 2024
Merged

feat: remove go aliases #7831

merged 11 commits into from
Jul 10, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jul 5, 2024

Completes the work from #6680 by removing the aliases, which were really tricky to work with from a code completion perspective.

By using #7759, we can avoid breaking the daggerverse, and just apply this for v0.12 and after.

With this change, all dagger types will need to be referenced directly from the internal/dagger package. Before:

package main

import (
	"context"
)

type MyModule struct{}

// Returns a container that echoes whatever string argument is provided
func (m *MyModule) ContainerEcho(stringArg string) *Container {
	return dag.Container().From("alpine:latest").WithExec([]string{"echo", stringArg})
}

After (see the change from Container to dagger.Container):

package main

import (
	"context"
	"dagger/my-module/internal/dagger"
)

type MyModule struct{}

// Returns a container that echoes whatever string argument is provided
func (m *MyModule) ContainerEcho(stringArg string) *dagger.Container {
	return dag.Container().From("alpine:latest").WithExec([]string{"echo", stringArg})
}

@jedevc jedevc added kind/breaking Breaking Change needs/changelog Pull requests that require a changelog entry labels Jul 5, 2024
@jedevc jedevc requested review from helderco, vito and sipsma July 5, 2024 13:12
@jedevc jedevc force-pushed the remove-go-aliases branch 2 times, most recently from a246f93 to 02075cc Compare July 5, 2024 13:45
@helderco
Copy link
Contributor

helderco commented Jul 5, 2024

I support this! It makes it more consistent with the other SDKs, generates less code, and makes the IDE happier because of no hidden docstrings in aliases.

@jedevc
Copy link
Member Author

jedevc commented Jul 5, 2024

AH! An issue I wasn't expecting to encounter - there are deps in our dependency tree (see shykes/daggerverse#10), which have random hashes as their engineVersion, which defaults to the latest.

This is fiddly, and not great. For now, we can probably make do by ensuring that our dependencies have good hashes anyways.

But long term, we really need to do #7760 - I'm going to add that to our milestone as well.

@jedevc jedevc added this to the v0.12.0 milestone Jul 5, 2024
@jedevc jedevc force-pushed the remove-go-aliases branch 5 times, most recently from 27cdab5 to 30d8c81 Compare July 9, 2024 11:16
@jedevc jedevc force-pushed the remove-go-aliases branch 2 times, most recently from 3791b42 to 967a3ce Compare July 9, 2024 13:23
@jedevc jedevc marked this pull request as ready for review July 9, 2024 13:23
@jedevc
Copy link
Member Author

jedevc commented Jul 9, 2024

Tests are failing due to dagger/dagger-test-modules#3.

@jedevc jedevc force-pushed the remove-go-aliases branch from 8f456ec to 30fe4ff Compare July 9, 2024 18:18
@jedevc
Copy link
Member Author

jedevc commented Jul 9, 2024

Hm, one more test is failing, TestModule/TestGoExtendCore - not quite sure why, will dig into this asap tomorrow. Otherwise, is ready for review.

@jedevc jedevc force-pushed the remove-go-aliases branch 5 times, most recently from f21cec7 to 3748836 Compare July 10, 2024 12:43
jedevc added 3 commits July 10, 2024 14:19

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the remove-go-aliases branch from 3748836 to 5fefd47 Compare July 10, 2024 13:21
@jedevc
Copy link
Member Author

jedevc commented Jul 10, 2024

I pointed https://github.com/vito/daggerverse` to my fork at https://github.com/jedevc/daggerverse-vito, so I could test with #7831.

@jedevc jedevc force-pushed the remove-go-aliases branch from 5fefd47 to 3b93a17 Compare July 10, 2024 13:40
jedevc and others added 7 commits July 10, 2024 14:40
But keep compat with older engine versions.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Without this, a `Container` type in a module called `Container` has 2
instances: `dagger.Container` and `Container`. These are unique, and
need to be treated as such.

This was resulting in a really bizarre error in TestModule/GoExtendCore
with the removal of aliases. Not *quite* sure why this only just
recently appeared.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Copy link
Contributor

@helderco helderco left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

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

Will be nice to skip the extra hop for go-to-definition, although I guess for that what I'd really want is some sort of magical go-to-backing-implementation feature (instead of the codegen'd client).

Also, I guess you could .-import if you preferred the syntax sugar from before?

Anyway, approving, didn't find anything wonky

Comment on lines +424 to +427
if basic.Kind() == types.Invalid {
s.Id("any")
break
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit seems interesting - is it to prevent codegen from blowing up on an unknown type? Maybe because it gets figured out on another pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, that's the behavior in c50cb10 (#7831). It produces invalid code, but the code wouldn't have compiled in the first place - this means dagger develop can at least succeed, so you can actually fix the issues, instead of having to just "guess".

@@ -385,23 +389,28 @@ package main

import (
"context"
"%[2]s/internal/dagger"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose now if I never run dagger develop and the generated module code isn't committed, instead of my editor giving me a bunch of errors for undefined types, there will just be this one unknown import, which is better

Copy link
Member Author

Choose a reason for hiding this comment

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

That too I guess! The main reason for this in here is that if you don't do it, go cleverly attempts to "find it" - and often seems to select dagger.io/dagger which is reasonably unhelpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that issue still exists but for dag right? I remember that auto-importing from somewhere. 🤔 Or is that fixed by now too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that issue exists for users if they don't run dagger develop first - since it's in dagger.gen.go. Sadly, I don't know about how to fix that one 🤔

@jedevc jedevc merged commit 9a89b2f into dagger:main Jul 10, 2024
47 of 51 checks passed
matipan pushed a commit to matipan/dagger that referenced this pull request Jul 10, 2024
* ci: avoid use of dagger package aliases

Signed-off-by: Justin Chadwell <me@jedevc.com>

* tests: update tests to avoid use of go aliases

Signed-off-by: Justin Chadwell <me@jedevc.com>

* chore: avoid codegen internal use of aliased types in go codegen

Signed-off-by: Justin Chadwell <me@jedevc.com>

* fix: go codegen should succeed even with invalid types

Signed-off-by: Justin Chadwell <me@jedevc.com>

* feat: remove go aliases

But keep compat with older engine versions.

Signed-off-by: Justin Chadwell <me@jedevc.com>

* chore: add changelog

Signed-off-by: Justin Chadwell <me@jedevc.com>

* fix: go should track fully qualified obj names

Without this, a `Container` type in a module called `Container` has 2
instances: `dagger.Container` and `Container`. These are unique, and
need to be treated as such.

This was resulting in a really bizarre error in TestModule/GoExtendCore
with the removal of aliases. Not *quite* sure why this only just
recently appeared.

Signed-off-by: Justin Chadwell <me@jedevc.com>

* chore: bump vito/daggerverse with correct engineVersions

Signed-off-by: Justin Chadwell <me@jedevc.com>

* Fix typos

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>

* Update wolfi

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>

* Update legacy test from latest merged PR

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>

---------

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Co-authored-by: Helder Correia <174525+helderco@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/breaking Breaking Change needs/changelog Pull requests that require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants