-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(KONFLUX-3187): adding test selection logic for repo infra-deploy… #1323
Conversation
Skipping CI for Draft Pull Request. |
26a44ae
to
b6d9fa1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing missing to be able to test this is the change in the magefile so that we can test/use against infra-deployments, right? Similar to this one
e2e-tests/magefiles/magefile.go
Line 328 in b6d9fa1
if openshiftJobSpec.Refs.Repo == "release-service-catalog" { |
} | ||
|
||
// Rule Catalog for the infra-deployments repo running specicfc suites | ||
var InfraDeploymentsTestFilesOnlyRule = rulesengine.Rule{Name: "E2E PR Test File Diff Execution", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the name and description here
|
||
// ExecuteInfraDeploymentsDefaultTestAction excutes all the e2e-tests and component suites | ||
func ExecuteInfraDeploymentsDefaultTestAction(rctx *rulesengine.RuleCtx) error { | ||
rctx.LabelFilter = "konflux-demo,release-service,jvm-build-service,image-controller,integration-service,ec,build-templates,multi-platform,build-service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want to run the konflux-demo in this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right,
re checked the related chart
fixed, just set the konflux-demo one .
// Rule Catalog for the infra-deployments repo running specicfc suites | ||
var InfraDeploymentsTestFilesOnlyRule = rulesengine.Rule{Name: "E2E PR Test File Diff Execution", | ||
Description: "Runs specific tests when test files are the only changes in the e2e-repo PR", | ||
Condition: rulesengine.All{rulesengine.Any{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All/Any
combination to capture the use case if a PR changes multiple files in their respective directory? I don't think you need the All
here.
})}} | ||
|
||
// CheckNoComponentsFilesChanged checks if in repo infra-deployments changed files other than components | ||
func CheckNoComponentsFilesChanged(rctx *rulesengine.RuleCtx) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two different patterns being used for the default rule and the diff files rule. Neither is right or wrong. This comment is more of a consistency of following the same pattern for both rules set. So this one is more of a nitpick. Since you've already written out the rules for each directory, you may want to convert the default rule into a RuleChain as well and us a None
conditional check around all the rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point ,
if I went to implement the default rule based on the RuleChain you mentioned,
would it it be like the suggested below ?
var InfraDeploymetsDefaultRuleChain = rulesengine.Rule{Name: "Infra Deployments Default Test Execution",
Description: "Run the Konflux-demo suite tests when an Infra-deployments PR includes changes to files outside of the specified components.",
Condition: rulesengine.None{&InfraDeploymentsTestFilesOnlyRule},
Actions: []rulesengine.Action{rulesengine.ActionFunc(ExecuteInfraDeploymentsDefaultTestAction)},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kasemAlem I was thinking something like the below. That is what I have in the example from the e2e-test repo
rulesengine.None{
rulesengine.ConditionFunc(CheckPkgFilesChanged),
rulesengine.ConditionFunc(CheckMageFilesChanged),
rulesengine.ConditionFunc(CheckCmdFilesChanged),
rulesengine.ConditionFunc(CheckNoFilesChanged),
rulesengine.ConditionFunc(CheckTektonFilesChanged),
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it,
I've replaced it to use the current rules with None ,
also tested
3b8f5a9
to
728e38a
Compare
agree, |
4026120
to
76e4f09
Compare
76e4f09
to
bc4a170
Compare
magefiles/magefile.go
Outdated
err = engine.MageEngine.RunRules(rctx, "tests", "infra-deployments") | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err = engine.MageEngine.RunRules(rctx, "tests", "infra-deployments") | |
if err != nil { | |
return err | |
} | |
return nil | |
return engine.MageEngine.RunRules(rctx, "tests", "infra-deployments") |
magefiles/magefile.go
Outdated
rctx.JobName = jobName | ||
rctx.JobType = jobType | ||
|
||
files, err := getChangedFiles() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this won't work. We need to get list of changed files from within the infra-deployments directory, but this function lists files from the current (e2e-tests) directory.
I'd suggest to update the getChangedFiles
func to accept the parameter for the location of the directory it should run git diff
against.
@Dannyb48 wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree,
it didn't work on the infra-deployments , so I've fixed the function
to get the repo and to check the absolute path of infra-deployments
tested and working fine , see examples in description of PR
bc4a170
to
942ecbe
Compare
fe75e77
to
6e2b912
Compare
return len(rctx.DiffFiles.FilterByDirGlob("components/integration/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/base/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/base/external-secrets/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/development/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/base/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/stone-prod-p01/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/stone-prod-p02/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/rh-certs/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/staging/base/*")) != 0 || | ||
len(rctx.DiffFiles.FilterByDirGlob("components/integration/staging/stone-stage-p01/*")) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think listing all subdirectories not ideal. If someone adds a new dir in one of the listed directories, the filter won't longer be valid.
It sucks that the "path/filepath" pkg doesn't support the double star (/**/*
) expression to include all subdirectories. But there's relatively easy workaround.
Let's import the doublestart package and update this line
e2e-tests/magefiles/rulesengine/types.go
Line 405 in 7e38b5e
if matched, _ := filepath.Match(filter, file.Name); !matched { |
with
if matched, _ := doublestar.PathMatch(filter, file.Name); !matched {
which is a method that implements the support for double star
If we do this, then the condition can look like:
return len(rctx.DiffFiles.FilterByDirGlob("components/integration/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/base/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/base/external-secrets/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/development/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/base/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/stone-prod-p01/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/production/stone-prod-p02/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/rh-certs/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/staging/base/*")) != 0 || | |
len(rctx.DiffFiles.FilterByDirGlob("components/integration/staging/stone-stage-p01/*")) != 0 | |
return len(rctx.DiffFiles.FilterByDirGlob("components/integration/**/*")) != 0 |
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I really like this! I've found the current standard implementation of filepath.match very limiting. If we implement this we can also clean up the logic in e2e-repo in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree ,
I've tried the below implementation and tested it locally , on several scenarios
and seems working fine :
var InfraDeploymentsBuildServiceTestFileChangeRule = rulesengine.Rule{Name: "Infra-deployments PR Build service component File Change Rule",
Description: "Map build service tests files when Build service component files are changed in the infra-deployments PR",
Condition: rulesengine.ConditionFunc(func(rctx *rulesengine.RuleCtx) bool {
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/**/*.yaml")) != 0 ||
len(rctx.DiffFiles.FilterByDirGlob("components/build-service/**/*.go")) != 0
}),
Actions: []rulesengine.Action{rulesengine.ActionFunc(func(rctx *rulesengine.RuleCtx) error {
AddLabelToLabelFilter(rctx, "build-service")
return nil
})}}
this one does not require importing import the doublestart package
but I'm fine with both methods , wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I really like this! I've found the current standard implementation of filepath.match very limiting. If we implement this we can also clean up the logic in e2e-repo in a separate PR
Awesome,
will start implementing with the double star
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be very surprised if this works with the standard library method, because they still have open issue about it here.
Could you try changing some deeply nested file, e.g. components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
and paste the log here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the implementation , tested locally several scenarios
working as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be very surprised if this works with the standard library method, because they still have open issue about it golang/go#11862.
Could you try changing some deeply nested file, e.g. components/build-service/base/build-pipeline-config/build-pipeline-config.yaml and paste the log here?
also tested with files README , OWNERS ..
➜ infra-deployments git:(test-infra) vim components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
➜ infra-deployments git:(test-infra) ✗ git status
On branch test-infra
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
no changes added to commit (use "git add" and/or "git commit -a")
➜ infra-deployments git:(test-infra) ✗ git add components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
➜ infra-deployments git:(test-infra) ✗ git commit -m "dasa"
[test-infra f20e20c0] dasa
1 file changed, 1 insertion(+)
➜ e2e-tests git:(KONFLUX-3187) ✗ ./mage -v local:runInfraDeploymentsRuleDemo
Running target: Local:RunInfraDeploymentsRuleDemo
exec: git "-C" "/home/kasemalem/integration-project/e2e-tests/tmp/infra-deployments" "diff" "--name-status" "upstream/main"
I0820 19:43:55.217851 24459 utils.go:473] The following files, components/build-service/base/build-pipeline-config/build-pipeline-config.yaml, were changed!
I0820 19:43:55.217902 24459 types.go:72] Loading the catalog for, infra-deployments, from category, tests
I0820 19:43:55.217918 24459 types.go:144] The following rules have matched Infra-deployments PR Components File Diff Execution.
I0820 19:43:55.217922 24459 types.go:157] DryRun has been enabled will apply them in dry run mode
exec: ginkgo "--seed=1724172235" "--dry-run" "--timeout=1h30m0s" "--grace-period=30s" "--output-interceptor-mode=none" "--label-filter=build-service,konflux" "--p" "--output-dir=." "./cmd" "--"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we implement this we can also clean up the logic in e2e-repo in a separate PR
@Dannyb48 @psturc I've created a task for implementing double star for the e2e-repo KONFLUX-4115
magefiles/magefile.go
Outdated
@@ -247,7 +247,7 @@ func (Local) CleanupPrivateRepos() error { | |||
repoNamePrefixes := []string{"build-e2e", "konflux", "multi-platform", "jvm-build-service"} | |||
quayOrgToken := os.Getenv("DEFAULT_QUAY_ORG_TOKEN") | |||
if quayOrgToken == "" { | |||
return fmt.Errorf("%s", quayTokenNotFoundError) | |||
return fmt.Errorf(quayTokenNotFoundError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking out these changes and rebasing should resolve this issue.
git pull --rebase <remote-name> main
9a12a63
to
fd821b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one question for Sushanta, so let's wait for him
otherwise lgtm
/approve
Description: "Map build-templates tests files when Build-templates component files are changed in the infra-deployments PR", | ||
Condition: rulesengine.ConditionFunc(func(rctx *rulesengine.RuleCtx) bool { | ||
|
||
return len(rctx.DiffFiles.FilterByDirGlob("components/build-templates/base/**/*")) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be "components/build-templates/**/*"
?
...or when I think about it, it would maybe make more sense to trigger build-templates
tests when the pipeline configmap is updated instead
@tisutisu wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psturc yes you are right. build-definitions changes are promoted through this ConfigMap, it would be good to run build-templates
labelled tests when build-pipeline-config.yaml
file is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then for build templates Rule the condition should be like this
return len(rctx.DiffFiles.FilterByDirGlob("components/build-templates/base/**/*")) != 0 | |
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/base/**/*")) != 0 |
if any changes was made in components/build-templates trigger any specific suite or the default konflux is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So according to what Sushanta mentioned above, I think we should trigger "build-templates" test, when the related config map is updated, i.e.
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/base/build-pipeline-config/*")) != 0
And since this would cover the build-templates
test scenario, I think we don't need another rule for components/build-templates
and its subdirectories, i.e. the default test suite (konflux
) should be run.
@tisutisu please correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psturc I am okay with this change we should trigger "build-templates" test, when the related config map is updated and don't need another rule for components/build-templates and its subdirectories, i.e. the default test suite (konflux) should be run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,
based on that ,
we should exclude this change from the components/build-service Rule , right ?
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/**/*")) != 0 &&
len(rctx.DiffFiles.FilterByDirGlob("components/build-service/base/build-pipeline-config/*")) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components/build-service rule will be there.. but needs to be modified..
- when this file
components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
changed, runbuild-templates
labeled tests - when changes in other sub-directories inside
components/build-service
except thatbuild-pipeline-config.yaml
, we can runbuild-service
labeled tests
}), | ||
rulesengine.ActionFunc(ExecuteTestAction)}} | ||
|
||
var InfraDeploymentsIntegrationTestFileChangeRule = rulesengine.Rule{Name: "Infra-deployments PR Integration component File Change Rule", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the late notice.. will it be good change the naming of the rules for infra-deployment
repo .. for readability improvement.. rename InfraDeploymentsIntegrationTestFileChangeRule
to something like InfraDeploymentsIntegrationComponentChangeRule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this,
@psturc wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to me
fd821b5
to
deb7783
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/**/*")) != 0 && | ||
len(rctx.DiffFiles.FilterByDirGlob("components/build-service/base/build-pipeline-config/*")) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't cover the case when someone would update
components/build-service/base/build-pipeline-config/*
- and some other files under
components/build-service/**/*
but I think that's so rare kind of update that we can address it later if we agree on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this scenario, we would expect that the labels be the three : build-service, build-templates, konflux
am I correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ran locally :
infra-deployments git:(test-infra) vim components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
➜ infra-deployments git:(test-infra) ✗ vim components/build-service/development/kustomization.yaml
➜ infra-deployments git:(test-infra) ✗ git status
On branch test-infra
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: components/build-service/base/build-pipeline-config/build-pipeline-config.yaml
modified: components/build-service/development/kustomization.yaml
no changes added to commit (use "git add" and/or "git commit -a")
➜ infra-deployments git:(test-infra) ✗ git add components/build-service/base/build-pipeline-config/build-pipeline-config.yaml components/build-service/development/kustomization.yaml
➜ infra-deployments git:(test-infra) ✗ git commit -m "dd"
[test-infra 654ae9bd] dd
2 files changed, 2 insertions(+)
Result:
➜ e2e-tests git:(KONFLUX-3187) ./mage -v local:runInfraDeploymentsRuleDemo
Running target: Local:RunInfraDeploymentsRuleDemo
exec: git "-C" "/home/kasemalem/integration-project/e2e-tests/tmp/infra-deployments" "diff" "--name-status" "upstream/main"
I0821 17:08:36.789232 823676 utils.go:473] The following files, components/build-service/base/build-pipeline-config/build-pipeline-config.yaml, components/build-service/development/kustomization.yaml, were changed!
I0821 17:08:36.789289 823676 types.go:72] Loading the catalog for, infra-deployments, from category, tests
I0821 17:08:36.789313 823676 types.go:144] The following rules have matched Infra-deployments PR Components File Diff Execution.
I0821 17:08:36.789319 823676 types.go:157] DryRun has been enabled will apply them in dry run mode
exec: ginkgo "--seed=1724249316" "--dry-run" "--timeout=1h30m0s" "--grace-period=30s" "--output-interceptor-mode=none" "--label-filter=build-templates,konflux" "--p" "--output-dir=." "./cmd" "--"
added a tests selection logic for the infra-deploymnents repo Signed-off-by: Kasem Alem <[email protected]> chore: Upgrade testing infrastructure secrets (konflux-ci#1314) * Upgrade testing infrastructure secrets * Fix to main branch and remove fake values --------- Co-authored-by: redhat-appstudio-qe-bot <[email protected]> Revert "chore: Upgrade testing infrastructure secrets (konflux-ci#1314)" This reverts commit b6d9fa1. Reapply "chore: Upgrade testing infrastructure secrets (konflux-ci#1314)" This reverts commit a60c13d. Reapply "chore: Upgrade testing infrastructure secrets (konflux-ci#1314)" This reverts commit 9b9c6fc.
deb7783
to
bf86e8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
var InfraDeploymentsBuildServiceTemplatesComponentChangeRule = rulesengine.Rule{Name: "Infra-deployments PR Build service component and templates File Change Rule", | ||
Description: "Map build service tests files when Build service component and templates files are changed in the infra-deployments PR", | ||
Condition: rulesengine.ConditionFunc(func(rctx *rulesengine.RuleCtx) bool { | ||
|
||
return len(rctx.DiffFiles.FilterByDirGlob("components/build-service/**/*")) != 0 && | ||
len(rctx.DiffFiles.FilterByDirGlob("components/build-service/base/build-pipeline-config/*")) != 0 | ||
}), | ||
Actions: []rulesengine.Action{rulesengine.ActionFunc(func(rctx *rulesengine.RuleCtx) error { | ||
AddLabelToLabelFilter(rctx, "build-service") | ||
return nil | ||
})}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unnecessary rule and will lead to applying build-service
label even when only files under components/build-service/base/build-pipeline-config/*
are changed.
But IMO this is a behavior we can live with for a while until we fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: psturc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
1- Examples from local Run :
in the tmp/infra-deployments created a new branch , edited README of components/integration , added committed
ran the DryRun , so we expect that the LabelFilter have both "integration-service" and "konflux-demo":
2- Example of local Run :
Changing only the README.md file in Infra-deployments folder so we expect that only the konflux-demo run
3- Example when no file was changed:
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
KONFLUX-3187
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: