concourse / concourse-pipeline-resource

!!! use the `set_pipeline` step instead !!!
https://concourse-ci.org/jobs.html#schema.step.set-pipeline-step.set_pipeline
MIT License
76 stars 42 forks source link

fix acceptance tests and move to go modules #64

Closed eedwards-sk closed 4 years ago

eedwards-sk commented 4 years ago

This is the first time I've done anything in golang, so be warned. I do lots of build engineering so the docker / gomod changes should be solid, though.

I'm working on bringing #16 up-to-date, but first I had to get existing acceptance tests passing. So this is that PR.


I tested against a local (5.3.0) concourse with no pipelines, and apparently that breaks some check and in tests (which expect the target concourse server to already have pipelines).

The in test, "target not provided" seemed to be broken from the start -- it was clearing the target, but then storing it in ATC_EXTERNAL_URL, which ends up being used anyway when target is unset. So the failure condition was never met.

While I was testing, I was focusing the tests or running them serially, which surfaced a couple tests that were actually relying on the environment state not being corrupted, and would "get lucky" when ran randomly, in parallel. So by resetting the environment at the start of each test, it solved these inconsistent state issues.


Finally, I moved the project to go1.13 with go modules. It's not a v2 module (because you've already tagged 2.x in the repo and it breaks if you try to v2 it), but it's now using go modules.

The only remaining thing that I think may need to be tweaked is build.yaml which is presumably used at concourse, and I didn't see any directions on testing it.

eedwards-sk commented 4 years ago

Looks like one of the tests fails due to the order of arguments changing. I'll take a stab at fixing that one, too.

eedwards-sk commented 4 years ago

Looks like the problem is in fly.go:

    for key, value := range vars {
        payload, err := json.Marshal(value)

        if err != nil {
            return nil, err
        }

        allArgs = append(allArgs, "-y", fmt.Sprintf("%s=%s", key, payload))
    }

maps do not preserve insertion order, and iterating over them using range reveals the order changes from run-to-run

I'm looking into a solution to either preserve insertion order of vars, or ideally simplify the test to not care.

eedwards-sk commented 4 years ago

Solved one of the non-determinant ordering issue tests by simply asserting on substrings rather than order.

There are likely more tests like that which may fail in the same manner due to range and map non-determinant ordering, so they can be addressed similarly as they're identified.

eedwards-sk commented 4 years ago

Is this repo no longer maintained?

izabelacg commented 4 years ago

Hey @eedwards-sk ! Sorry for taking such a long time to get to your PR. Would you still be interested in working on this? If so, would you mind breaking the changes into 2 separate PRs? Migrating to go.mod shouldn't cause the tests to break. So, we could have one PR for just migrating to go mod and another, for all the other changes.

izabelacg commented 4 years ago

Hey @eedwards-sk , thanks for submitting this PR. At first I had requested it to be split into 2 PRs (or at least 2 separate commits: one for the tests fix and the other for using go mod), but I understand that it might not be in your radar anymore. However, I think your contribution is valuable to the resource, so I'll merge it. Thank you again for the contribution and sorry for the huge delay.