Skip to content
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.

Sign up for GitHub

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

Jump to bottom

*: pre-transition ign refactor #610

Merged
merged 4 commits into from
May 17, 2019
Merged

*: pre-transition ign refactor #610

merged 4 commits into from
May 17, 2019

Conversation

kikisdeliveryservice
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice commented Apr 9, 2019 •
edited
Loading

- What I did
Now that we have more time, I am breaking up PR #583 . This PR is removing the changes from #583 that can go into the code-base now to make the future transition easier.

This PR makes the following changes:

  • update helpers.go NewIgnConfig() to use MaxVersion and sub igntypes for ignv2_2types as package alias
  • for all non-test files, substitute igntypes for ignv2_2types as package alias (these changes are trivial but take up a lot of space in the PR, let's do it now!)
  • in test files, make the above substitution, and also refactor to use NewIgnConfig() instead of generating empty config and hardcoding version.
  • minor doc update to reflect above changes

After this lands, the future subtantive ignition change PR should be clearer and easier to test and evaluate.

- How to verify it
This PR makes no functional changes and everything should work exactly the same as before.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 9, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] pre-transition ign cleanup [WIP] pre-transition ign refactor Apr 9, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] pre-transition ign refactor [WIP] *: pre-transition ign refactor Apr 9, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] *: pre-transition ign refactor *: pre-transition ign refactor Apr 9, 2019
@openshift-ci-robot 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 Apr 9, 2019
LorbusChris
LorbusChris reviewed Apr 9, 2019
View reviewed changes
Copy link
Member

@LorbusChris LorbusChris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found a minimal nit

lib/resourceapply/machineconfig_test.go Show resolved Hide resolved
@runcom runcom mentioned this pull request Apr 9, 2019
Merged
@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Apr 10, 2019 •
edited
Loading

seeing:

Apr 09 23:39:44 ip-10-0-3-216 bootkube.sh[8203]: Skipped /v1, Resource=secrets as it already exists

and in Cluster Operators

"name": "cloud-credential",
                    {
                        "lastTransitionTime": "2019-04-09T23:21:23Z",
                        "message": "4 of 4 credentials requests are failing to sync.",
                        "reason": "CredentialsFailing",
                        "status": "True",
                        "type": "Failing"
                    },

/retest

@kikisdeliveryservice
Copy link
Contributor Author

Not going to retest right now PR is hitting BZ# 1698253

see artifacts/e2e-aws-op/pods.json:
"message": "Failed to inspect image \"registry.svc.ci.openshift.org/ci-op-0d9qlb6n/stable@sha256:8eb140d803ec324f5b3b472be7ffc2a6c582923833280c9d78227ccfb65d2154\": rpc error: code = Unknown desc = Manifest does not match provided manifest digest sha256:8eb140d803ec324f5b3b472be7ffc2a6c582923833280c9d78227ccfb65d2154",

@kikisdeliveryservice
Copy link
Contributor Author

/test e2e-aws

@runcom
Copy link
Member

runcom commented Apr 10, 2019

/retest

@kikisdeliveryservice
Copy link
Contributor Author

aws vpc issues, this is a known issue, going to hold off on retesting.

@kikisdeliveryservice
Copy link
Contributor Author

ok trying again:

/test e2e-aws

@kikisdeliveryservice
Copy link
Contributor Author

this is green ping @runcom

@runcom
Copy link
Member

runcom commented Apr 12, 2019

I fear this goes under:

Tech debt, refactors or code cleanups should not be
merged at this point.

See eparis email about feature freeze, This will surely help us on moving towards ignv2s3 but since it's not scheduled for 4.1/4.2, this should go in in 4.2 like others e.g. #600.

@runcom runcom added the 4.2 label Apr 12, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2019
@kikisdeliveryservice kikisdeliveryservice force-pushed the ign-cleanup branch 2 times, most recently from 04cedd7 to 42041fc Compare May 14, 2019 19:00
@runcom
Copy link
Member

runcom commented May 15, 2019

needs another rebase

@kikisdeliveryservice kikisdeliveryservice changed the title *: pre-transition ign refactor [wip] *: pre-transition ign refactor May 15, 2019
@openshift-ci-robot 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 May 15, 2019
@kikisdeliveryservice
Copy link
Contributor Author

want to look over the rebased changes a bit.

@kikisdeliveryservice
ign test refactor
3c37f50 
- update tests to remove ign version from package alias
- replace ign config delclarations with hardcoded versions
with calls to NewIgnConfig()
@kikisdeliveryservice kikisdeliveryservice changed the title [wip] *: pre-transition ign refactor *: pre-transition ign refactor May 15, 2019
@openshift-ci-robot 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 May 15, 2019
@kikisdeliveryservice
Copy link
Contributor Author

/test e2e-aws-upgrade

@kikisdeliveryservice
Copy link
Contributor Author

ci woes...

/retest

@kikisdeliveryservice
Copy link
Contributor Author

/refresh

@kikisdeliveryservice
Copy link
Contributor Author

ok retesting again...

/retest

@kikisdeliveryservice
Copy link
Contributor Author

/retest

@kikisdeliveryservice
Copy link
Contributor Author

/test e2e-aws-upgrade

@kikisdeliveryservice
Copy link
Contributor Author

@runcom finally green and fully rebased!

@runcom
Copy link
Member

runcom commented May 17, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kikisdeliveryservice, runcom

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 [kikisdeliveryservice,runcom]

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

@openshift-merge-robot openshift-merge-robot merged commit 7beb99c into openshift:master May 17, 2019
@kikisdeliveryservice kikisdeliveryservice deleted the ign-cleanup branch May 17, 2019 16:04
@cgwalters cgwalters mentioned this pull request Jun 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LorbusChris LorbusChris LorbusChris left review comments

@runcom runcom Awaiting requested review from runcom

Assignees

@runcom runcom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kikisdeliveryservice @runcom @openshift-ci-robot @LorbusChris @openshift-merge-robot