IGNORE ME #2
Open
IGNORE ME #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
from
69b6bac
to
00387e0
Compare
abayer
force-pushed
the
esi-test-changes
branch
3 times, most recently
from
7918f6d
to
7f043a0
Compare
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
from
00387e0
to
a967fd0
Compare
This change is to minimize the size of the actual implementation. We need to change a number of tests in `pkg/reconciler/pipelinerun/pipelinerun_test.go` to be table-based, so that we can test behavior under each possible value for the new `embedded-status` feature flag. Here, we just modify the relevant tests to be table-based and use a parameterized helper function for the actual execution, with a test case for each value. It also adds helper functions for checking the relevant fields in the `PipelineRun` status, which are hard-coded to always handle the current, "full" embedded status approach. This also splits out `TestUpdatePipelineRunStatusFromTaskRuns` and `TestUpdatePipelineRunStatusFromRuns` into a separate file, `pipelinerun_updatestatus_test.go`. When the TEP-0100 implementation lands, this will also contain additional tests for updating via child references. Splitting like this helps keep `pipelinerun_test.go` from getting even more bloated than it is currently. Until the implementation, these table-based tests are purely duplicative - they're going to run the same and check the same things for any value of `embedded-status`, but the implementation PR will be cleaner, only adding the implementation and changing the helper functions to take all of the possible "embedded-status" values into account. The changes which need to be made in the implementation PR are all marked with `// TODO(abayer): ...` in `pipelinerun_test.go` and `pipelinerun_updatestatus_test.go`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
5 times, most recently
from
13fd232
to
2fea896
Compare
This doesn't address all uses of explicitly declared structs in `pipelinerun_test.go`, but it's progress. I decided that it made sense to do this incrementally, rather than attempting a single Big Bang switching all of them over. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
This still retains some explicit struct declaration in a few tests, and there are some cases which could be rewritten to be simpler, but this was just a bulk porting from explicit struct->YAML parsing, and is not a "finished" product. That means there will be followups refining things, etc. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
https://github.blog/2022-04-12-git-security-vulnerability-announced/ was announced and then fixed in Git 2.35.2. This ends up creating issues like actions/checkout#760, where the directory the git repo lives isn't owned by the same user performing the git operations. This does appear to be affecting us - see https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4750/pull-tekton-pipeline-integration-tests/1514143139193950210/ for example, which has the telltale error message of `fatal: unsafe repository ('/workspace/go/src/github.com/GoogleContainerTools/skaffold' is owned by someone else)`. This is an attempt to fix that by having `git-init` call `git config --global --add safe.directory [repo dir]` before fetching, etc. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
This comes out of discussions on tektoncd#4739 - with the new minimal embedded status changes which will be introduced in that PR, we can see that we're currently using the output of `pipelineRunFacts.State.GetTaskRunsStatus(pr)` and `pipelineRunFacts.State.GetRunsStatus(pr)` for two separate purposes: * To set `pr.Status.TaskRuns` and `pr.Status.Runs` with the full embedded status * To pass to `resources.ApplyTaskResultsToPipelineResults` for populating results It's understandable why `ApplyTaskResultsToPipelineResults` is using the maps from `pr.Status.[TaskRuns|Runs]`, since those maps do contain everything needed for propagating results up from the tasks to the pipeline run, but if you look at the current implementation, you can see that it's shuffling the maps around into a different form that's more suited for what it's doing than the original form. So this PR reworks `ApplyTaskResultsToPipelineResults` to instead take maps in the form the current implementation uses internally, with new functions on `PipelineRunState` to get these new maps without needing to use the `pr.Status.[TaskRuns|Runs]` form as an intermediary. This makes the pre-minimal-embedded-status implementation cleaner, and is particularly helpful in that regard once we do have minimal embedded status in place. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
from
e886963
to
bbb42ee
Compare
This change proposes that breaking changes may be introduced to the Go client libraries, as long as existing yaml/json CRD definitions remain unaffected. Breaking changes to the Go client libraries do not affect the Dashboard team, as the Dashboard team no longer uses these libraries. The CLI team has decided that the impact of such changes on them is acceptable.
This doesn't switch everything - the "global" tasks and other resources are left as is, for example. But it does move most of the explicit `Task` and `TaskRun` structs to parsed YAML instead. Tangentially related to tektoncd#4610. =) Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
…d status This builds on tektoncd#4694, tektoncd#4734, and tektoncd#4753. It will feed into a revamped tektoncd#4739, all as part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md and tektoncd#3140. Specifically, this adds functionality to `pkg/reconciler/pipelinerun/resources` in `pipelinerunresolution.go` and `pipelinerunstate.go` which will be needed for the full implementation. These changes won't have any effects in the current situation, because `pr.Status.ChildReferences` is never populated, so can be made independently of the rest of the implementation, thus also shrinking the size of the rest of the implementation PR(s) for easier review. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
from
bbb42ee
to
7990ce4
Compare
And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (tektoncd#4705, tektoncd#4734, tektoncd#4753, tektoncd#4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#4753 * tektoncd#4757 * tektoncd#4760 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
embedded-statuses-impl-mk2
branch
from
7990ce4
to
ce12e81
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…pelineRuns
See:
This implements TEP-0100, building on top of the flags/fields/docs changes in tektoncd#4705.
Signed-off-by: Andrew Bayer andrew.bayer@gmail.com