USHIFT-852: assets.yaml declaring manifests to copy from releases by pmtk · Pull Request #1347 · openshift/microshift

pmtk · 2023-02-09T17:18:43Z

Adds asset.yaml that lists all the files in assets/. Focused only on sourcing the assets (either copying from STAGING_DIR or using git restore). It does not include any modifications (like yq) to the manifests (yet).
Its purpose is to have common place for both rebase procedure and rebase presubmit.

Introduces new way of copying manifests from staging dir based on contents of assets.yaml file.
Commands such as cp, git restore, rm -rf are removed from rebase.sh's functions dealing with manifests (assets/).

Other changes include:

wrapping {{ .template }} inside quotes to get correct yaml file, so many sed commands are removed
formatting lvms/topolvm manifests with oc create --dry-run=client (includes sorting) (a consequence of using oc create --dry-run on all lvms/topolvm manifests, not just the copied ones)
changing method of creating "lvmd" ConfigMap from templating to overriding ConfigMap's data (because templating "{{ .lvmd }}" would keep the " resulting in incorrect data)
removed Test_renderLvmdConfig because it was testing approach based on renderTemplate() which is no longer used

openshift-ci-robot · 2023-02-09T17:18:46Z

@pmtk: This pull request references USHIFT-852 which is a valid jira issue.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ggiguash · 2023-02-10T06:02:51Z

/retest

@pmtk, can you add the fix description to the PR? There seem to be some formatting changes, so it's not clear what needs to be reviewed.

openshift-ci-robot · 2023-02-10T09:03:23Z

@pmtk: This pull request references USHIFT-852 which is a valid jira issue.

Details

In response to this:

Adds asset.yaml that lists all the files in assets/. Focused only on sourcing the assets (either copying from STAGING_DIR or using git restore). It does not include any modifications (like yq) to the manifests (yet).
Its purpose is to have common place for both rebase procedure and rebase presubmit.

Introduces new way of copying manifests from staging dir based on contents of assets.yaml file.
Commands such as cp, git restore, rm -rf are removed from rebase.sh's functions dealing with manifests (assets/).

Other changes include:

wrapping {{ .template }} inside quotes to get correct yaml file

formatting lvms manifests with oc create --dry-run=client (includes sorting)

changing method of creating "lvmd" ConfigMap from templating to overriding ConfigMap's data

removed Test_renderLvmdConfig because it was testing approach based on renderTemplate() which is no longer used

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2023-02-10T09:07:14Z

@pmtk: This pull request references USHIFT-852 which is a valid jira issue.

Details

In response to this:

Adds asset.yaml that lists all the files in assets/. Focused only on sourcing the assets (either copying from STAGING_DIR or using git restore). It does not include any modifications (like yq) to the manifests (yet).
Its purpose is to have common place for both rebase procedure and rebase presubmit.

Introduces new way of copying manifests from staging dir based on contents of assets.yaml file.
Commands such as cp, git restore, rm -rf are removed from rebase.sh's functions dealing with manifests (assets/).

Other changes include:

wrapping {{ .template }} inside quotes to get correct yaml file, so many sed commands are removed

formatting lvms/topolvm manifests with oc create --dry-run=client (includes sorting) (a consequence of using oc create --dry-run on all lvms/topolvm manifests, not just the copied ones)

changing method of creating "lvmd" ConfigMap from templating to overriding ConfigMap's data (because templating "{{ .lvmd }}" would keep the " resulting in incorrect data)

removed Test_renderLvmdConfig because it was testing approach based on renderTemplate() which is no longer used

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pmtk · 2023-02-10T09:08:06Z

@pmtk, can you add the fix description to the PR? There seem to be some formatting changes, so it's not clear what needs to be reviewed.

I copied (and slightly tweaked) descriptions from individual commits

pmtk · 2023-02-10T09:08:50Z

/cc @copejon
PTAL on the lvms/topolvm related changes

dhellmann

I like the direction this is going.

dhellmann · 2023-02-10T20:57:51Z

pkg/components/render_test.go

What happened to this test?

I removed it because it was testing if the ConfigMap is properly templated ({{ .lvmd }}) but now it's substituted so this test didn't made much sense:

# pkg/components/storage.go - if err := assets.ApplyConfigMaps(cm, renderTemplate, lvmdRenderParams, kubeconfigPath); err != nil { + if err := assets.ApplyConfigMapWithData(cm, map[string]string{"lvmd.yaml": lvmdRenderParams["lvmd"].(string)}, kubeconfigPath); err != nil {

I think, if we'd want to test something, it would be assets.ApplyConfigMapWithData() (although I think it would need a refactor to split this logic from applying it to API)

OK, it makes sense to remove this. We can add other tests in separate PRs if we want them.

dhellmann · 2023-02-10T21:06:26Z

scripts/auto-rebase/handle-assets.py

Suggested change

if 'no_clean' in dir and dir['no_clean']:

if dir.get('no_clean', False):

dhellmann · 2023-02-10T21:06:57Z

scripts/auto-rebase/handle-assets.py

Suggested change

if "files" in dir:

for f in dir['files']:

for f in dir.get('files', []):

dhellmann · 2023-02-10T21:07:48Z

scripts/auto-rebase/handle-assets.py

This can be the same as for files.

pmtk · 2023-02-13T18:16:15Z

/test e2e-reboot

dhellmann

When I tried to run this version of the rebase script it replaced some of the assets with "Script exited with error.". Maybe you can prepare a separate PR showing the output of this work?

dhellmann · 2023-02-13T18:30:31Z

pkg/components/render_test.go

OK, it makes sense to remove this. We can add other tests in separate PRs if we want them.

dhellmann · 2023-02-13T18:39:46Z

When I tried to run this version of the rebase script it replaced some of the assets with "Script exited with error.". Maybe you can prepare a separate PR showing the output of this work?

It looks like that's the same problem with oc requiring credentials that we've discussed elsewhere.

pmtk · 2023-02-14T08:13:37Z

When I tried to run this version of the rebase script it replaced some of the assets with "Script exited with error.". Maybe you can prepare a separate PR showing the output of this work?

Sure.
I ran contents of #1362 last_rebase.sh on this branch.
Here's the branch it produced: https://github.com/pmtk/microshift/tree/rebase-4.13.0-0.nightly_amd64-2023-02-13-194759_arm64-2023-02-14-014641 I created PR against rebase branch: #1365 although the diff is weird because it shows the same changes like it would be against main branch

Here's PR between today's rebase PR and same refs produced by this branch #1367 (although the diff looks like it's against main, maybe if I'd rebase my branch on top of CI PR branch the diff would be "correct" - but you can see same changes with some reformatting).

Edit2: Here's PR between 1362 and 1367 rebased on top of 1362: #1368

dhellmann · 2023-02-14T15:26:23Z

When I tried to run this version of the rebase script it replaced some of the assets with "Script exited with error.". Maybe you can prepare a separate PR showing the output of this work?

Sure. I ran contents of #1362 last_rebase.sh on this branch. Here's the branch it produced: https://github.com/pmtk/microshift/tree/rebase-4.13.0-0.nightly_amd64-2023-02-13-194759_arm64-2023-02-14-014641 I created PR against rebase branch: #1365 although the diff is weird because it shows the same changes like it would be against main branch

Here's PR between today's rebase PR and same refs produced by this branch #1367 (although the diff looks like it's against main, maybe if I'd rebase my branch on top of CI PR branch the diff would be "correct" - but you can see same changes with some reformatting).

Edit2: Here's PR between 1362 and 1367 rebased on top of 1362: #1368

OK, that all looks good. This needs a rebase, but then I'll LGTM it.

asset.yaml that lists all the files in assets/. It does not include any modifications to the manifests. Focused only on sourcing the assets (either copying from STAGING_DIR or using git restore). Its purpose is to have common place for both rebase procedure and rebase presubmit.

introduces new way of copying manifests from staging dir based on contents of assets.yaml file

changes include: - wrapping {{ .template }} inside quotes to get correct yaml file - formatting lvms manifests with `oc create --dry-run=client` (includes sorting) - changing method of creating "lvmd" ConfigMap from templating to overriding ConfigMap's data

removed Test_renderLvmdConfig because it was testing approach based on renderTemplate() which is no longer used

dhellmann · 2023-02-14T18:39:59Z

/retest

dhellmann

/lgtm

openshift-ci · 2023-02-14T18:41:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhellmann,pmtk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 9, 2023

openshift-ci bot requested review from dhellmann and ggiguash February 9, 2023 17:19

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2023

openshift-ci bot requested a review from copejon February 10, 2023 09:08

pmtk force-pushed the rebase/recipe branch 2 times, most recently from 37dd718 to aaf1461 Compare February 10, 2023 13:49

pmtk mentioned this pull request Feb 10, 2023

USHIFT-377: make sure all files from assets/ are handled by rebase #1351

Merged

dhellmann reviewed Feb 10, 2023

View reviewed changes

pmtk force-pushed the rebase/recipe branch 2 times, most recently from f3edc57 to 2976005 Compare February 13, 2023 13:57

dhellmann reviewed Feb 13, 2023

View reviewed changes

pmtk force-pushed the rebase/recipe branch from 2976005 to 1ee4bad Compare February 14, 2023 08:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023

pmtk mentioned this pull request Feb 14, 2023

USHIFT-377: MicroShift Rebase presubmit openshift/release#36269

Closed

pmtk force-pushed the rebase/recipe branch from 1ee4bad to f6857d3 Compare February 14, 2023 16:04

pmtk added 4 commits February 14, 2023 17:04

handle-assets.py

d38103f

introduces new way of copying manifests from staging dir based on contents of assets.yaml file

use handle-assets.py in rebase.sh

9050924

pmtk added 3 commits February 14, 2023 17:04

align tests with new way to create lvmd configmap

4442d27

removed Test_renderLvmdConfig because it was testing approach based on renderTemplate() which is no longer used

check rebase.sh preconditions and install yq

4f8175e

fix wrong schema and add missing files

523cdde

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023

pmtk force-pushed the rebase/recipe branch from f6857d3 to 523cdde Compare February 14, 2023 16:04

dhellmann reviewed Feb 14, 2023

View reviewed changes

openshift-ci bot assigned dhellmann Feb 14, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2023

openshift-merge-robot merged commit 276be93 into openshift:main Feb 14, 2023

pmtk deleted the rebase/recipe branch March 29, 2023 13:26

This was referenced Jul 5, 2024

NO-ISSUE: rebase-main-4.17.0-0.nightly-2024-07-04-162615_amd64-2024-07-04_arm64-2024-07-04 #3589

Closed

NO-ISSUE: rebase-main-4.17.0-0.nightly-2024-07-07-131215_amd64-2024-07-07_arm64-2024-07-07 #3600

Merged

	if 'no_clean' in dir and dir['no_clean']:
	if dir.get('no_clean', False):

	if "files" in dir:
	for f in dir['files']:
	for f in dir.get('files', []):

Conversation

pmtk commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 9, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggiguash commented Feb 10, 2023

Uh oh!

openshift-ci-robot commented Feb 10, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 10, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmtk commented Feb 10, 2023

Uh oh!

pmtk commented Feb 10, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk commented Feb 13, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Feb 13, 2023

Uh oh!

pmtk commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Feb 14, 2023

Uh oh!

dhellmann commented Feb 14, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pmtk commented Feb 9, 2023 •

edited

Loading

openshift-ci-robot commented Feb 9, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 10, 2023 •

edited by openshift-ci bot

Loading

pmtk commented Feb 14, 2023 •

edited

Loading