Fix merge integration test

[MDrakos]

DX-3115 Nightly failure: TestPullIntegrationTestSuite/TestMergeBuildScript

[MDrakos]

This update is to follow the same structure as here: https://github.com/ActiveState/cli/blob/master/pkg/platform/model/buildplanner/build.go#L113

It appears we want to the buildscript at_time to be the commit at_time even if the buildscript has it's own time set.

[Naatan]

I think we should fix this issue closer to the root. The issue here is that UnmarshalBuildExpression is apparently overriding the AtTime. We should fix that.

We might still be setting at time in the raw representation, we should move that out of raw and just set it on the buildscript struct itself. Raw effectively represents the build expression, which does not contain the at time at all.

[mitchell-as]

There was a time before we had "$at_time" in build expressions. In those days, it was a timestamp. When we find a timestamp, we should be using it, not overriding it with a later atTime. I don't agree with the latest change, but maybe I'm missing something.

[MDrakos]

As per our discussion at stand the at_time value being a timestamp in the buildexpression is a legacy use case that will be only be on old commits.

This change ensures that the at_time in the build script will always be the commit at_time and we disregard the at_time we find in the solve node.

[Naatan]

Reading the code now, I realize that this should just be an update to our testing code. The reason the test was failing is we were essentially testing how we want things to work with new buildscripts, but it was failing cause it was dealing with an old builscript. The solution with old buildscripts was appropriate: Take the at_time value, and record it on the buildscript level. Next time the user commits either it is bumped or at the very least it will be recorded on the commit level.

So sorry for going back on what we discussed during stand; but I think the fix here ought to be no code changes to anything but the integration test. And the integration test fix in theory would just be to bump the time stamp on the commit that it starts from for the comparison (ie. so it uses a build expression without the at time value hardcoded on it).

[MDrakos]

From our discussion the following points are how we want to handle the at_time value along with how this solution should address them.

• We respect the hardcoded at_time value
  • This is done via storing the at_time found in the solve node separately from the preferred time value.
  • We return the commit at_time if that was set externally, otherwise we default to the solve at time.
  • When we unmarshall the build expression the solve at_time could either be a time stamp or a reference.
• BUT; any future commits we make transfer it to the commit level and use a variable instead
  • This is already handled by our current implementation
  • The buildscript library updates the at_time in the solve node to be a reference if it finds a timestamp there.
  • When we make a commit we supply the at_time as part of the input parameters which sets the commit's at_time
• If at_time is already used as a variable then we don't do anything special other than respect the commit at time and not touch the build expression value
  • This is done by updating the SetAtTime() and AtTime() methods to work with the preferred (commit) atTime and not the one found in the solve node if set.
  • SetAtTime() is currently being called when we unmarshall the buildexpression from a commit response which sets the atTime value to the commit at time.
• I as a developer shouldn't need to think about this when using the buildscript library externally
  • Since SetAtTime() is working with a separate value than the solve node external callers of the library can be assured that calling SetAtTime() with a time value will be preserved and returned when calling AtTime()

[Naatan]

I don't think this is meeting the criteria:

• We respect the hardcoded at_time value

Because we are always returning atTime when it is set. Which will always be the case. I think you are not seeing this in your tests because you are likely still setting atTime before unmarshalling.

Essentially the following test should pass (pseudo code):

func TestLegacyAtTime(t *testing.T) {
  bs := newBuildScript()
  err := bs.UnmarshalBuildExpression(bexp)
  require.NoError(t, err)
  err := bs.SetAtTime(bogusValue)
  require.ErrorAs(err, ErrAtTimeAlreadySet)
  require.Equal(t, bs.AtTime(), buildExpressionAtTime)
}

So that would be asserting that the build expression atTime is preserved and overrides the commit atTime.

The error check on SetAtTime is just an in-the-moment solution to the problem of "I as a developer do not need special knowledge". Because when working with legacy atTime values it straight up isn't appropriate to override the at time with that of the commit. I think erroring out in this case is ok, because then at least the failure will be obvious to the developer and they can then handle it better (eg. ignore that type of error and move on).

[MDrakos]

@Naatan You're right I misinterpreted that point. I didn't realize we want to retain the solve node's atTime. Going through the code there appears to be some cases where we want to override the atTime like when we pass the --ts flag or when merging. I've added the ability to specify overriding the time value but we may want to go with a different pattern.

[Naatan]

I think for the install action we always want to override.

[MDrakos]

Ah okay, so anytime we make a change we want to update the timestamp? I was under the impression that we want to preserve the timestamp in the solve node and I was only bumping it when the user explicitly passed the --ts flag.

[Naatan]

We only want to preserve it when we retrieve the commit. Once we're acting on the buildscript (ie. making changes) the existing timestamp should always be overwritten.