Skip to content
This repository was archived by the owner on Jun 14, 2019. It is now read-only.

Conversation

@bbguimaraes
Copy link
Contributor

@bbguimaraes bbguimaraes commented Mar 20, 2019

  • Changes to cluster profiles are identified.
  • Jobs that use those profiles are chosen for rehearsal.
  • Integration test verifies the above (see note below).
  • ConfigMap manager is changed to also create cluster profiles.
  • Jobs are changed to use those CMs.
  • More thorough unit testing.

Follow-up:

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2019
@bbguimaraes bbguimaraes changed the title rehearse: detect cluster profile changes [WIP] rehearse: detect cluster profile changes Mar 20, 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 Mar 20, 2019
@bbguimaraes
Copy link
Contributor Author

pull-ci-openshift-ci-operator-prowgen-master-unit
--- FAIL: TestIntegration (0.07s)

/me confused

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 21, 2019
@bbguimaraes
Copy link
Contributor Author

Only a few minor items left. The existing bits are unlikely to change, so this is in a reviewable state. I've removed the integration test bits for now to reduce the noise.

/cc @openshift/developer-productivity-test-platform

Copy link
Member

@droslean droslean left a comment

Choose a reason for hiding this comment

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

Nice start, But I don't like the idea of getting the cluster profile differences from git diff. I would prefer just comparing the bytes of the file. Also, for the cm name hash, we can get the hash from the cluster-profile's data.

@bbguimaraes
Copy link
Contributor Author

Fixed an issue with the version of git we use in CI (1.8.3.1, which is now six years old) not recognizing lower-case letters as exclusion filters in --diff-filter.

@openshift-ci-robot openshift-ci-robot 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 Mar 22, 2019
Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Everything here looks pretty good -- what's left?

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2019
@bbguimaraes
Copy link
Contributor Author

Updated.

@stevekuznetsov
Everything here looks pretty good -- what's left?

See the description. I separated the three conditions that already existed before this PR. Those do not interfere with the code here and should be dealt with in separate PRs, IMO.

Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

What's left for this to be WIP? Even your description checklist says it's complete and I agree the remaining items can be done as followups.

To me it looks good (btw I really like your code, reads nice and I like the fact that adding a feature are 99% additions and not changes).

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 27, 2019
@bbguimaraes
Copy link
Contributor Author

Why, thank you =)

The WIP was just to give people a chance to check the commits addressing the latest review. I will squash the history and remove the label.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2019
@bbguimaraes bbguimaraes changed the title [WIP] rehearse: detect cluster profile changes rehearse: detect cluster profile changes Mar 28, 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 Mar 28, 2019
@stevekuznetsov
Copy link
Contributor

/hold cancel
/lgtm
:shipit:

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 28, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbguimaraes, petr-muller, stevekuznetsov

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 [bbguimaraes,petr-muller,stevekuznetsov]

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 5a3fc31 into openshift:master Mar 28, 2019
@bbguimaraes bbguimaraes deleted the cluster_profiles branch March 29, 2019 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

Development

Successfully merging this pull request may close these issues.

6 participants