feat: Remove magic strings from Source types

[pmengelbert]

This is a draft PR. It won't pass the CI since I have not yet modified the tests.

@cpuguy83 , can you take a look and let me know if this is roughly what you had in mind for the data structures?

Reference #50

[cpuguy83]

Maybe add something like source.processArgs(args)?

[pmengelbert]

• Add source.processArgs(...)

[pmengelbert]

This is done.

[pmengelbert]

One other question for when you come back -- is Cmd a source variant, or is an option for sources that have some kind of shell environment available?

I'm thinking it should be a subfield on, for example, SourceDockerImage and SourceContext. Thoughts?

[cpuguy83]

Yes, sub field for SourceDockerImage. On December 15, 2023, GitHub ***@***.***> wrote: One other question for when you come back -- is Cmd a source, or is an option for sources that have some kind of shell environment available? I'm thinking it should be a subfield on, for example, SourceDockerImage or SourceContext. Thoughts?

[pmengelbert]

Yes, sub field for SourceDockerImage.

Only that field? Theoretically someone could provide an oci-layout as a build context, right? In that case, you could probably run a command also. Should I bother with that? If so, are there any other variants that should have the Cmd subfield?

[cpuguy83]

Lovely how me email client is so verbose :)

Only that field? Theoretically someone could provide an oci-layout as a build context, right?

They could but we don't really account for that today.
I wouldn't worry about it right now as that's a pretty advanced case.

[pmengelbert]

• Fix autogenerated content (changing field names)
• Troubleshoot e2e tests
• Add unit tests for new private source methods

[adamperlin]

Can we make a SourceKind interface that all of these implement? It seems like that could be useful

[pmengelbert]

Can we make a SourceKind interface that all of these implement? It seems like that could be useful

I thought the same thing, but after discussion with Brian, we decided against it. That discussion is worth reading. It really made us both think hard about when to use an interface vs when not to. The tldr is:

• No interface keeps the spec closer to simple data - makes things easier to reason about, especially when you're first perusing the code base.
• The spec is a configuration file with a set of instructions, and should be "dumb" and not "smart"
• Unmarshaling the config is much easier when it's just simple data
• We still have to do switch statements to convert the source into the interface any time we need to reference or set the underlying data (ex when we do the string substitution for the build args).
• In general, it's better not to fight go's lack of enums and unions. I personally wish the language had them, but the designers of the language made the decision not to implement them. Go's designers had a lot of opinions about how to keep the language simple (for example, error handling is manually handled everywhere; or the only possible loop is a for loop). Fighting against the grain can make things pretty awkward (even more awkward than the switch/case statements).
• An interface is generally better suited to situations where
  • The call-site is not concerned with potential side-effects within the interface's implementation(s).
  • The behavior belongs to a separate package or an unrelated type
  • A failure to implement an interface on [a type that's added later] causes a compile-time error. That wouldn't happen in this case: say we have a SourceKind field on Source, and we add a new SourceKind, but forget to make it satisfy the interface. We only catch the error if we remember to test it, which is never a given, because humans are humans.

With all that said, I'm curious how you would see the interface functioning here. Can you think of an implementation that would actually buy us something for the trouble? It's quite possible I just had the wrong shape in mind when I was thinking of how to do it.

[adamperlin]

Wow, thank you for your detailed response here! I went back and read the original thread, and it was an interesting read. I agree with many of the points.

For the sake of discussion, I will explain what I was thinking, even though it perhaps isn't a perfect fit.

---

Instead of adding a SourceKind field to Source, we can keep the fields separate (as they are now) and add a helper method to upcast them into an interface when we select which one is set (I think this is fairly similar to what you were suggesting in one of your comments in the thread).

However, the upcasting would be done in such a way that we could get checks at compile time to ensure that each new source kind implements the SourceKind interface.

Here is an example of what I was thinking:

type SourceKind interface {
    Present() bool // this is perhaps one of the hackier parts, basically returns whether or not we have a nil interface,
                           // and has to be implemented for each source kind
    LLB() ...
    SetRef()
    ...
}

Then GetSource() looks like:

func (s *Source) GetSource() (SourceKind, error) {
	found := false
	var use SourceKind = nil
       // if a variant does not implement our interface, we will not be able to put it in the array here
      // without a compile-time error
	for _, s := range []SourceKind{src.DockerImage, src.HTTPS, ... /*add any new variants here*/ } {
		if s.Present() && found {
			return nil, errors.New("multiple variants set")
		} else if s.Present() {
			found = true
			use = s
		}
	}

	if use == nil {
		return nil, errors.New("all nil")
	}

	return use, nil
}

Then, anywhere we would use a switch statement, we use GetSource to select the proper variant. For example,

func (s *Source) NeedsToSelectSourceKind() {
      sourceToUse, err := s.GetSource()
      if err != nil {
          // handle error, these are good sanity checks
      }
      
       // do something with source to use


    }

This eliminates the switch statements and adds built-in checking to ensure that the set of source fields are behaving like a union (so this simplifies the duplicates/unset checking in my view). Also, anytime we add a new source, we only need to add it in one place to check that it implements the right functionality: we simply update the GetSource method to include that source in its array of options, and we will get compile-time errors if it doesn't satisfy the SourceKind interface.

---

That being said, I am kind of torn since the methods that would be added to SourceKind are sort of inappropriate. I.e., if SourceKind is part of the spec, as Brian was saying, it makes more sense to have a function that takes in the spec and creates llb state based on it, rather than having multiple smaller implementations delegated as methods defined ON a SourceKind. It is weird to have a spec that generates the llb state, it's a spec, not a state creator.

[cpuguy83]

One thing we could do is on each of the pointer types implement something like

// naming is hard
type AsLLBState interface {
  AsState(p string, includes, excludes []string) (llb.State)
  IsDir() bool
}

Then the switch effectively becomes:

func GetSource(s *Spec, src Source, name string) (st llb.State, isDir bool, err error) {
  // validate all the things

  // load the source
  switch {
  case src.Build != nil:
    return src.Build.AsState(), src.Build.IsDir(), nil
  // other cases
  default:
    panic("something is wrong")
  }
}

I'd be fine with this.

[pmengelbert]

After working on this a bit, we found that this works better.

// naming is hard
type AsLLBState interface {
  AsState(p string, includes, excludes []string) LLBGetter
  IsDir() bool
}

[adamperlin]

My biggest high-level feedback so far is to ask if we can encapsulate the different sources under an interface, say SourceKind. In my opinion, that would make some of the code around handling the different source cases cleaner, as the cases would be split up into separate implementations of interface methods.

[adamperlin]

Wow, thank you for your detailed response here! I went back and read the original thread, and it was an interesting read. I agree with many of the points.

For the sake of discussion, I will explain what I was thinking, even though it perhaps isn't a perfect fit.

---

Instead of adding a SourceKind field to Source, we can keep the fields separate (as they are now) and add a helper method to upcast them into an interface when we select which one is set (I think this is fairly similar to what you were suggesting in one of your comments in the thread).

However, the upcasting would be done in such a way that we could get checks at compile time to ensure that each new source kind implements the SourceKind interface.

Here is an example of what I was thinking:

type SourceKind interface {
    Present() bool // this is perhaps one of the hackier parts, basically returns whether or not we have a nil interface,
                           // and has to be implemented for each source kind
    LLB() ...
    SetRef()
    ...
}

Then GetSource() looks like:

func (s *Source) GetSource() (SourceKind, error) {
	found := false
	var use SourceKind = nil
       // if a variant does not implement our interface, we will not be able to put it in the array here
      // without a compile-time error
	for _, s := range []SourceKind{src.DockerImage, src.HTTPS, ... /*add any new variants here*/ } {
		if s.Present() && found {
			return nil, errors.New("multiple variants set")
		} else if s.Present() {
			found = true
			use = s
		}
	}

	if use == nil {
		return nil, errors.New("all nil")
	}

	return use, nil
}

Then, anywhere we would use a switch statement, we use GetSource to select the proper variant. For example,

func (s *Source) NeedsToSelectSourceKind() {
      sourceToUse, err := s.GetSource()
      if err != nil {
          // handle error, these are good sanity checks
      }
      
       // do something with source to use


    }

This eliminates the switch statements and adds built-in checking to ensure that the set of source fields are behaving like a union (so this simplifies the duplicates/unset checking in my view). Also, anytime we add a new source, we only need to add it in one place to check that it implements the right functionality: we simply update the GetSource method to include that source in its array of options, and we will get compile-time errors if it doesn't satisfy the SourceKind interface.

---

That being said, I am kind of torn since the methods that would be added to SourceKind are sort of inappropriate. I.e., if SourceKind is part of the spec, as Brian was saying, it makes more sense to have a function that takes in the spec and creates llb state based on it, rather than having multiple smaller implementations delegated as methods defined ON a SourceKind. It is weird to have a spec that generates the llb state, it's a spec, not a state creator.

[pmengelbert]

Looks like mounts aren't implemented correctly yet.

• Troubleshoot Sources.[].DockerImage.Cmd.Mounts implementation.

[pmengelbert]

• Fix docs/examples specs.

[pmengelbert]

@cpuguy83 @adamperlin This is ready for a real review. The spec changes are fully passing the tests and the examples are up to date. I will make a pass through tomorrow to add any unit tests that may be necessary.

I will also take a look at the code organization, as there may be some things commented out or out of place.

[cpuguy83]

The sub function usage seems like a bit of a run-around with pointers.
In all cases we can do something like:

s.Git.URL, err = lex.ProcessWordWIthMap(s.Git.URL, args)

[pmengelbert]

done

[cpuguy83]

Suggested change

	switch count {
	case 0:
	errs = goerrors.Join(errs, fmt.Errorf("no non-nil source variant"))
	case 1:
	// success condition
	default:
	errs = goerrors.Join(errs, fmt.Errorf("more than one source variant defined"))
	}
	return errs
	switch {
	case 0:
	return goerrors.Join(errs, fmt.Errorf("no non-nil source variant"))
	case 1:
	return nil
	default:
	return goerrors.Join(errs, fmt.Errorf("more than one source variant defined"))
	}

[pmengelbert]

Ah, the reason I didn't do this is because I check the mounts and the source types. If there are multiple errors I thought it might be useful to return all of them. Instead, I've changed it to report errors in the mounts immediately, and then added the above.

[pmengelbert]

(done)

[adamperlin]

Maybe consider placing in a separate method, since this case is more involved

[pmengelbert]

done

[adamperlin]

Can we perhaps name this function something a little more specific? Maybe substituteArgs or similar?

[pmengelbert]

done