#not yet, sorry 😦 looking into this now

1 messages · Page 1 of 1 (latest)

opal ibex
#

if there is anything I can do to help, kindly let me know. and thank you for all your help so far to me.

red pewter
opal ibex
#

incase we decide to approve the PR for merge, I am working on verifying the last added integration test to ensure that it works for all the configured modules

red pewter
alpine seal
#

i still have no idea what the impact on backwards-compat is 😛
it's potentially a breaking change, with no module compat:

  • either we need to add that,
  • or be okay with users saying that the release that includes this may have breaking changes, and might break things in the daggerverse - i think we need to run it on all the modules in the daggerverse (though we don't need to do this in CI every run, I think we need that data)
#

i think we're likely okay though 🙂

#

i have a suspicion that the only names this opens up, are ones that had internal conflicts inside dagger - so no one has published modules with this

#

if we ever change implementation after this though, we will almost definitely need to do the module compat work (i think this is unlikely we need to do this)

opal ibex
alpine seal
#

yeah, the code looks good, but i would like someone to confirm on more than 10% of the daggerverse

opal ibex
#

(I also started building a standalone dagger module that folks can run on demand)

#

yeah, the code looks good, but i would like someone to confirm on more than 10% of the daggerverse
yep, i am doing that right now. i have pulled list of modules from daggerverse for that, and iterating through a sample of modules from that list

alpine seal
#

it could go in modules

#

genuinely, that would be epic, i would love that

#

i think we should have it as that instead of a ModuleSuite test, but that can be a follow-up

opal ibex
#

👍 I will let you know as soon as I have something working for that

red pewter
#

that's great! Thank you both! 🫂

opal ibex
#

it seems like we found our first breakage:

github.com/Excoriate/daggerverse/module-template@7cbec9fbfc9c62f2e41ee0471b97861984c090f1

2054:     [1m3.4s] |                     Diff:
2054:     [1m3.4s] |                     --- Expected
2054:     [1m3.4s] |                     +++ Actual
2054:     [1m3.4s] |                     @@ -8794,3 +8794,3 @@
2054:     [1m3.4s] |                           (string) (len=12) "isDeprecated": (bool) false,
2054:     [1m3.4s] |                     -     (string) (len=4) "name": (string) (len=13) "doHttprequest",
2054:     [1m3.4s] |                     +     (string) (len=4) "name": (string) (len=13) "doHttpRequest",
2054:     [1m3.4s] |                           (string) (len=4) "type": (map[string]interface {}) (len=3) {
2054:     [1m3.4s] |                     @@ -8897,3 +8897,3 @@
2054:     [1m3.4s] |                           (string) (len=12) "isDeprecated": (bool) false,
2054:     [1m3.4s] |                     -     (string) (len=4) "name": (string) (len=13) "doJsonapicall",
2054:     [1m3.4s] |                     +     (string) (len=4) "name": (string) (len=13) "doJsonapiCall",
2054:     [1m3.4s] |                           (string) (len=4) "type": (map[string]interface {}) (len=3) {

I'll verify more modules, and create a list

red pewter
#

cc @flint glacier

alpine seal
#

hm, if there's a breaking change, i think we should try and do the module compat, and hand-off to the old implementation when the dagger.json for that module is older than a specific version

#

it's a pretty miserable change, but i think this proves we should do it, breaking the daggerverse is pretty bad

opal ibex
#

but this mean if there are multiple modules with different engine version in dagger.json (e.g. as a dependency of one module), we may get into a mix and match sort of problem?

alpine seal
#

yeah, module compat handles this fine

#

it just means carefully propagating it around

red pewter
opal ibex
#

that is a good suggestion. I will complete the test first and report back

alpine seal
#

but if we're deliberately introducing breaking changes, we probably should do it in v0.13.0

opal ibex
#

I've created a module for this, and started the test for all the daggerverse modules. (there are about 750 of them). I will report back once the test completes. (running with parallel=10)

opal ibex
#

update: in last about 2 hrs,

total ran: 41,
OK: 25,
error: 16,
confirmed-compatibility issue: 5

At this rate this will run for a day it seems. (I am looking into what could be improved in the module/script, currently I am triggering dagger call for each module as a separate process)

#

at this stage I think it makes sense to start looking at compat support for this.

(Please let me know if you think otherwise.)

alpine seal
#

yeah i think that's right 👍

#

what i would probably do (just some suggestions!):

  • write a LegacySuite test case to check the old/new behavior for different dagger.jsons
  • then, go through all calls to strcase, and mark which ones have to do with user-supplied names (i don't think this is an issue for the core api, so it doesn't need to propagate to dagql or cmd/dagger package for example)
  • then work out how to propagate the module version around - you could pass it in directly, you could pass in a strcase.Caser or similar object, or maybe even propagate it in the context? Kind of up to you IMO, whatever feels natural.
#

One thing to note though - we need to decide whether this is influrnced by the caller's version, or the callee's version:

  • e.g. if a v0.13.0 (future release with this patch in) module calls a v0.11.9 (past release without this) dependency, should the casing of the calls to it be according to v0.13.0 or v0.11.9?
    There's good arguments both ways I think, but I think it's easier to have it be determined by the dependency module - otherwise, we're fully going into "the view of a module depends on the caller", which is possible, but maybe a lot more complicated.
#

this function gets called when a module registers a function - so we need to take the engineVersion for that module, and somehow get it to here, so we can switch between implementations

#

does that make some amount of sense? sorry for the text wall, i think a lot of the context for the module compat is mostly in my head, and i've not really explained how we would expand it's scope like this before

opal ibex
#

no, this is very useful. thank you

alpine seal
#

one thing that i would mention - however you choose to propagate, it would be nice if it was reusable in the future. i can imagine in the future that we might want to change some other aspect of the registration api based on engineVersion, so would be good if we could preserve that in some way

#

maybe the View function might also be interesting to you

opal ibex
#

one thing I am a bit worried about it adding this complexity to dagger. do we anticipate these kind of breaking changes in future as well (especially around naming?). I think (if possible) we should standardize on one naming convention scheme (with good amount of tests around it) and try not to make version specific assumptions. but I also do not have a lot of context around other areas in the source code, so I understand that my view is quite limited here as well

#

maybe the View function might also be interesting to you
👍

alpine seal
#

maybe not around naming (hopefully!), but we might change certain registration apis, etc

opal ibex
#

would it be ok if I open a separate PR for adding this new module to check compatibility for reviews and maybe get that merged.

alpine seal
#

on a quick glance, it actually feels fairly positive - we need to modify the registration apis, essentially the core/schema typedefs, and maybe the gqlformat.go functions.
then each sdk needs this - python/elixir/co look easy, the bit of a trickier one is go - but thankfully propagation of the engineVersion for the sdks is already done, it just needs to be used

alpine seal
opal ibex
#

on a quick glance, it actually feels fairly positive
awesome, thank you. i will try this out.

opal ibex
#

just an update: I have something working, but not with dynamic loading based on engine version. I should have something working tonight/tomorrow morning for review.

opal ibex
#

Hi @alpine seal , if you have some time today, would it be possible for you to pair with me on this for some time? I have some of this working (e.g. atleast one module which failed earlier is working now), but I still have more failures, and it will be great to verify the changes so far, and also be able to make some progress using your expertise.

alpine seal
#

yeah sure!

#

i'm about to head to lunch in a moment, but could do after then? in say an hour/hour-and-a-half?

#

if you push to a branch, i'm also happy to provide feedback async

opal ibex
#

if possible (and not too much of a bother for you), I would really like to pick your brain about the progress and next steps.

alpine seal
#

yup, of course!

#

i could also do literally right now if that works better]

opal ibex
#

i can do right away too

alpine seal
alpine seal
#

once it's in, i might investigate automating it in our internal infra, and add it to our pre-release flight checks 😄

opal ibex
#

awesome. thank you for your feedback. I will get that incorporated and update the PR

#

another good news. the compat mode seems to be working as expected now 🎉 using the view as engine version

#

now I just need to cleanup the PR, and implement the changes to ignore base schema drift.