pkg/asset: remove asset stocks. store asset state in asset itself. #344

staebler · 2018-09-27T02:50:31Z

This improves the code surrounding accessing state for dependent assets. It reduces the amount of casting and de-serialization of state data.

Every asset is now associated with a unique type. The asset store retains at most a single instance of a given asset type.

crawford · 2018-09-27T17:31:06Z

/hold

Waiting on CI to actually test this code.

crawford · 2018-09-28T04:07:07Z

/hold cancel

crawford · 2018-09-28T14:35:27Z

/test all

wking · 2018-09-28T18:05:10Z

The router didn't come up in time:

Waiting for router to be created ...
NAME                          STATUS    ROLES       AGE       VERSION
ip-10-0-13-150.ec2.internal   Ready     master      1h        v1.11.0+d4cacc0
ip-10-0-14-104.ec2.internal   Ready     bootstrap   1h        v1.11.0+d4cacc0
ip-10-0-23-100.ec2.internal   Ready     master      1h        v1.11.0+d4cacc0
ip-10-0-39-16.ec2.internal    Ready     master      1h        v1.11.0+d4cacc0
Waiting for router to be created ...
Another process exited
2018/09/28 17:55:55 Container test in pod e2e-aws failed, exit code 1, reason Error

Sometimes that happens as a flake.

/retest

abhinavdahiya · 2018-09-28T18:10:01Z

/approve

abhinavdahiya · 2018-09-28T19:00:02Z

#344 (comment)

This might have been because the workers didn't come up. And the router is deployed on workers.

crawford · 2018-09-28T19:16:17Z

@staebler Can you fix the format of your commit title and add a description to the body? It helps the reviewer understand why things are being changed and it helps our future selves remember why we made the change.

crawford · 2018-09-28T19:32:49Z

pkg/asset/parents.go

+type Parents map[reflect.Type]*State
+
+// Get gets the state of the specified asset.
+func (p Parents) Get(asset Asset) (*State, bool) {


Why is there a distinction between Asset and State? Is it possible for each asset to implement its own special logic and the Asset interface, allowing us to drop State? I had imagined this method would look as follows:

func (p Parents) Get(asset &Asset) { // this isn't quite right because Asset is an interface *asset, ok := p[reflect.TypeOf(asset)] if !ok { panic("Asset %s was requested, but was not available", asset.Name()) } }

We could then pass in the subset of assets that have been declared by each assets' Dependencies() method to ensure that every asset is properly declaring its dependencies.

yifan-gu · 2018-10-03T19:19:45Z

@staebler Any updates on this one? Trying to figure out how #374 and my asset deserializationwork will fit with this one.

staebler · 2018-10-03T19:45:28Z

@yifan-gu I am working through some minor bugs. I'll push up an update with its current rough state.

docs/design/assetgeneration.md

abhinavdahiya · 2018-10-04T18:09:00Z

@staebler is it possible to split 18e53d1 into smaller commits for ease of review we can squash them back before we merge? WDYT?

staebler · 2018-10-04T18:57:57Z

@staebler is it possible to split 18e53d1 into smaller commits for ease of review we can squash them back before we merge? WDYT?

@abhinavdahiya I can break into a commit per package, but they will not be atomic commits. Is that sufficient? It makes it more cumbersome to resolve merge conflicts when there are multiple commits .If it makes the review easier, then I'll do it.

pkg/asset/ignition/bootstrap/bootstrap.go

abhinavdahiya · 2018-10-04T20:17:17Z

pkg/asset/tls/BUILD.bazel

@@ -0,0 +1,52 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")


*.bazel no bazel files required. can you remove this.

Yep, this got left in accidentally in one of the merges. I will remove it.

abhinavdahiya · 2018-10-04T20:24:35Z

@staebler this looks good, just some small not importants.
And discussion around why new interface WriteableAsset ?

abhinavdahiya · 2018-10-04T20:25:05Z

And sorry for the rebase around #400 :(

yifan-gu

/approve

yifan-gu · 2018-10-04T21:46:11Z

@crawford Wanna take another look on this?

abhinavdahiya · 2018-10-05T03:02:18Z

/approve

abhinavdahiya · 2018-10-05T16:47:18Z

operator-framework/operator-lifecycle-manager#500
seems to have broken us previously.

/retest

crawford · 2018-10-05T16:49:35Z

pkg/asset/asset.go

+// PersistToFile writes all of the files of the specified asset into the specified
+// directory. If the asset is not a WritableAsset, then no files will be written.
+func PersistToFile(asset Asset, directory string) error {
+	writableAsset, ok := asset.(WritableAsset)


Why not just change the signature of PersistToFile? Targets shouldn't include non-writable assets.

That is fine, too. I'll make that change.

crawford · 2018-10-05T17:12:50Z

/approve

This looks fantastic!

wking · 2018-10-05T17:57:08Z

pkg/asset/asset_test.go

+
+	asset := &persistAsset{}
+	expectedFiles := map[string][]byte{}
+	err = PersistToFile(asset, dir)


govet:

# github.com/openshift/installer/pkg/asset pkg/asset/asset_test.go:45:21: cannot use asset (type *persistAsset) as type WritableAsset in argument to PersistToFile: *persistAsset does not implement WritableAsset (missing Files method)

crawford · 2018-10-05T18:55:33Z

/lgtm

This improves the code surrounding accessing state for dependent assets. It reduces the amount of casting and de-serialization of state data. Every asset is now associated with a unique type. The asset store retains at most a single instance of a given asset type.

…asset state Assets have been changed to store their state internally.

If PersistToFile only accepts WritableAsset instances, then PersistToFile does not have to case to WritableAsset or deal with assets that do not generate any files.

crawford · 2018-10-05T20:18:46Z

/lgtm

openshift-ci-robot · 2018-10-05T20:18:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, staebler, yifan-gu

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:

~~OWNERS~~ [abhinavdahiya,crawford,staebler,yifan-gu]

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2018

openshift-ci-robot requested review from smarterclayton and steveej September 27, 2018 02:50

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2018

wking mentioned this pull request Sep 28, 2018

pkg/asset/installconfig: Add _CI_ONLY_STAY_AWAY_OPENSHIFT_INSTALL_AWS_USER_TAGS #364

Merged

crawford reviewed Sep 28, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2018

yifan-gu mentioned this pull request Oct 1, 2018

pkg/asset: Introduce Load() into the Asset interface that loads assets (from disk) #374

Merged

staebler changed the title ~~Remove asset stocks~~ WIP: Remove asset stocks Oct 3, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2018

staebler changed the title ~~WIP: Remove asset stocks~~ pkg/asset: remove asset stocks. store asset state in asset itself Oct 4, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2018

staebler changed the title ~~pkg/asset: remove asset stocks. store asset state in asset itself~~ pkg/asset: remove asset stocks. store asset state in asset itself. Oct 4, 2018

abhinavdahiya reviewed Oct 4, 2018

View reviewed changes

docs/design/assetgeneration.md Show resolved Hide resolved

rajatchopra mentioned this pull request Oct 4, 2018

Use string as the asset map key instead of the object itself #416

Closed

abhinavdahiya reviewed Oct 4, 2018

View reviewed changes

pkg/asset/ignition/bootstrap/bootstrap.go Show resolved Hide resolved

abhinavdahiya reviewed Oct 4, 2018

View reviewed changes

yifan-gu approved these changes Oct 4, 2018

View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 5, 2018

crawford reviewed Oct 5, 2018

View reviewed changes

abhinavdahiya mentioned this pull request Oct 5, 2018

Save assets in a state file on disk #388

Merged

wking reviewed Oct 5, 2018

View reviewed changes

openshift-ci-robot assigned crawford Oct 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2018

staebler added 3 commits October 5, 2018 15:53

doc/design: update asset generation design doc to reflect removal of …

fc83a2d

…asset state Assets have been changed to store their state internally.

pkg/asset: only accept WritableAsset to PersistToFile

2d3073f

If PersistToFile only accepts WritableAsset instances, then PersistToFile does not have to case to WritableAsset or deal with assets that do not generate any files.

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2018

openshift-merge-robot merged commit ee28106 into openshift:master Oct 5, 2018

wking mentioned this pull request Oct 5, 2018

cmd/openshift-install: Log Terraform errors when not in debug mode #423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/asset: remove asset stocks. store asset state in asset itself. #344

pkg/asset: remove asset stocks. store asset state in asset itself. #344

staebler commented Sep 27, 2018 •

edited

Loading

crawford commented Sep 27, 2018

crawford commented Sep 28, 2018

crawford commented Sep 28, 2018

wking commented Sep 28, 2018

abhinavdahiya commented Sep 28, 2018

abhinavdahiya commented Sep 28, 2018

crawford commented Sep 28, 2018

crawford Sep 28, 2018

yifan-gu commented Oct 3, 2018

staebler commented Oct 3, 2018

abhinavdahiya commented Oct 4, 2018

staebler commented Oct 4, 2018

abhinavdahiya Oct 4, 2018

staebler Oct 5, 2018

abhinavdahiya commented Oct 4, 2018

abhinavdahiya commented Oct 4, 2018 •

edited

Loading

yifan-gu left a comment

yifan-gu commented Oct 4, 2018

abhinavdahiya commented Oct 5, 2018

abhinavdahiya commented Oct 5, 2018 •

edited

Loading

crawford Oct 5, 2018

staebler Oct 5, 2018

crawford commented Oct 5, 2018

wking Oct 5, 2018

crawford commented Oct 5, 2018

crawford commented Oct 5, 2018

openshift-ci-robot commented Oct 5, 2018

		@@ -0,0 +1,52 @@
		load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

pkg/asset: remove asset stocks. store asset state in asset itself. #344

pkg/asset: remove asset stocks. store asset state in asset itself. #344

Conversation

staebler commented Sep 27, 2018 • edited Loading

crawford commented Sep 27, 2018

crawford commented Sep 28, 2018

crawford commented Sep 28, 2018

wking commented Sep 28, 2018

abhinavdahiya commented Sep 28, 2018

abhinavdahiya commented Sep 28, 2018

crawford commented Sep 28, 2018

crawford Sep 28, 2018

Choose a reason for hiding this comment

yifan-gu commented Oct 3, 2018

staebler commented Oct 3, 2018

abhinavdahiya commented Oct 4, 2018

staebler commented Oct 4, 2018

abhinavdahiya Oct 4, 2018

Choose a reason for hiding this comment

staebler Oct 5, 2018

Choose a reason for hiding this comment

abhinavdahiya commented Oct 4, 2018

abhinavdahiya commented Oct 4, 2018 • edited Loading

yifan-gu left a comment

Choose a reason for hiding this comment

yifan-gu commented Oct 4, 2018

abhinavdahiya commented Oct 5, 2018

abhinavdahiya commented Oct 5, 2018 • edited Loading

crawford Oct 5, 2018

Choose a reason for hiding this comment

staebler Oct 5, 2018

Choose a reason for hiding this comment

crawford commented Oct 5, 2018

wking Oct 5, 2018

Choose a reason for hiding this comment

crawford commented Oct 5, 2018

crawford commented Oct 5, 2018

openshift-ci-robot commented Oct 5, 2018

staebler commented Sep 27, 2018 •

edited

Loading

abhinavdahiya commented Oct 4, 2018 •

edited

Loading

abhinavdahiya commented Oct 5, 2018 •

edited

Loading