Skip to content

Conversation

@ahardin-rh ahardin-rh added this to the Next Release milestone Feb 22, 2021
@ahardin-rh ahardin-rh self-assigned this Feb 22, 2021
@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 2021
@netlify
Copy link

netlify bot commented Feb 22, 2021

Deploy preview for osdocs ready!

Built with commit 344aa99

https://deploy-preview-29673--osdocs.netlify.app

@ahardin-rh ahardin-rh force-pushed the cmp-operator-updates branch from ab972d0 to 2d22b19 Compare March 10, 2021 21:56
Copy link

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

nice!!

@ahardin-rh ahardin-rh force-pushed the cmp-operator-updates branch from ff9c0fc to d2a33a4 Compare March 31, 2021 19:29
@ahardin-rh ahardin-rh changed the title [WIP]Updating Compliance Operator docs Updating Compliance Operator docs Mar 31, 2021
@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 31, 2021
@ahardin-rh ahardin-rh requested a review from pdhamdhe-zz March 31, 2021 20:54
@ahardin-rh
Copy link
Contributor Author

@pdhamdhe Please provide QE review. Thank you!

Copy link

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Everything looks great! Just did a small comment and I think this is good to go.

@ahardin-rh ahardin-rh force-pushed the cmp-operator-updates branch from d2a33a4 to ce8dab1 Compare April 5, 2021 20:00
@ahardin-rh ahardin-rh added the peer-review-needed Signifies that the peer review team needs to review this PR label Apr 5, 2021
Copy link
Contributor

@xiaojiey xiaojiey Apr 6, 2021

Choose a reason for hiding this comment

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

for the subscription, there is an important field called "channel":
a. Run the following command to set the OpenShift Container Platform major and minor version as an environment variable, which is used as the channel value in the next step.
OC_VERSION=$(oc version -o yaml | grep openshiftVersion | grep -o '[0-9]*[.][0-9]*' | head -1)
b. Create the Subscription object YAML file by running:

----
apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: compliance-operator-sub
  namespace: openshift-compliance
spec:
  channel: "${OC_VERSION}"
  installPlanApproval: Automatic
  name: compliance-operator
  source: redhat-operators
  sourceNamespace: openshift-marketplace
----

Copy link

@pdhamdhe-zz pdhamdhe-zz Apr 6, 2021

Choose a reason for hiding this comment

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

It would be good if we mention channel while creating subscription but it is not mandatory. We can create subscription without mentioning channel and operator will get deployed using defaultChannel

$ oc get packagemanifest compliance-operator -oyaml |grep defaultChannel
  defaultChannel: "4.6"

Copy link
Contributor

@xiaojiey xiaojiey Apr 6, 2021

Choose a reason for hiding this comment

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

@pdhamdhe If works for 4.6. What if a 4.7 cluster? It will have 2 packagemanifests(one for 4.6 and the other one for 4.7). If so, for fresh install, it is better to recommend users to use OCP version instead.

Copy link

@pdhamdhe-zz pdhamdhe-zz Apr 6, 2021

Choose a reason for hiding this comment

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

@xiaojiey Jfyi.. By default there will be only one packagemanifest for each ocp version for operator. So on 4.7 cluster, the compliance operator packagemanifest will only have defaultChannel : "4.7" . I was able to installed operator on both the cluster i.e 4.6 & 4.7 using the subscription yaml file which is mentioned in documentation. Though, I am fine with both the methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

‘ProfileBundle’ is much reasonable, as it is a CR name.

Copy link

@pdhamdhe-zz pdhamdhe-zz Apr 6, 2021

Choose a reason for hiding this comment

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

No need to apply the file if we are creating namespace resource using $ oc create -f <file-name>.yaml and step 2 can be merged in step 1

@aburdenthehand aburdenthehand removed their request for review April 6, 2021 09:31

Choose a reason for hiding this comment

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

This is ScanSetting object. In point 5 & 6, it is mentioned scan setting binding, please make correction.

Choose a reason for hiding this comment

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

Also ScanSetting runs scans at 01:00 everyday not periodically every hour.

Choose a reason for hiding this comment

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

It would be good if we add -n openshift-compliance in all command mentioned in this section.

Choose a reason for hiding this comment

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

Correct command would be :
$ oc get compliancecheckresults -l 'compliance.openshift.io/check-status=FAIL,compliance.openshift.io/automated-remmediation

Choose a reason for hiding this comment

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

Also here, please update the selector to compliance.openshift.io/check-status=FAIL

Copy link
Contributor

@lbarbeevargas lbarbeevargas left a comment

Choose a reason for hiding this comment

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

This reworking of the Compliance Operator section looks great. I left feedback for general guidelines adherence and minimalism/simplification purposes.

It looks like there is a mix of command lead-in sentences (this inconsistency is present through the OCP docs), so I gave some varying feedback depending on the assembly. Examples:

To list all failing checks that can be remediated automatically , run:
Search for any outdated remediations:
Create a namespace object YAML file by running:

For the sake of minimalism, I think the second example above works best.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/Compliance Operator Scans/Compliance Operator scans/

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful in this intro sentence to say "Compliance Operator."
s/Before you can use the Operator, you must ensure it is deployed in the cluster./Before you can use the Compliance Operator, you must ensure that it is deployed in the cluster./

Copy link
Contributor

Choose a reason for hiding this comment

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

In the "not installed successfully" section below, it says to "Navigate to the Operators -> Installed Operators page." Is it important in this first verification step to also include Operators in the navigation directions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to specify the "Compliance Operator" here in case there are other Operators installed.
s/the Operator/the Compliance Operator/

Copy link
Contributor

Choose a reason for hiding this comment

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

s/namespace/Namespace/

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a Procedure module? If so, you can add .Procedure with a single bullet (or two bullets because this isn't necessarily a sequence).

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a Procedure or Concept module? If Procedure, you can add .Procedure and then have bullets for each potential check? If Concept, I think the rest of the module looks okay as is, but you could change the title to something like "Filters for failed compliance check results".

Copy link
Contributor

Choose a reason for hiding this comment

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

s/To list checks that belong to a specific suite, run:/List checks that belong to a specific suite:/

Similar rewording suggestion applies to the other commands in this module (for minimalism and consistency across the Compliance Operator book).

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good candidate to split the command into two lines with \.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good candidate to split the command into two lines with \.

@lbarbeevargas lbarbeevargas added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 6, 2021
@ahardin-rh ahardin-rh force-pushed the cmp-operator-updates branch 2 times, most recently from 874812b to 25ea12b Compare April 8, 2021 20:36
@ahardin-rh ahardin-rh force-pushed the cmp-operator-updates branch from 25ea12b to 344aa99 Compare April 8, 2021 20:48
@ahardin-rh ahardin-rh merged commit e0ffb51 into openshift:master Apr 8, 2021
@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.6

@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.7

@ahardin-rh
Copy link
Contributor Author

/cherrypick enterprise-4.8

@openshift-cherrypick-robot

@ahardin-rh: new pull request created: #31436

Details

In response to this:

/cherrypick enterprise-4.6

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-cherrypick-robot

@ahardin-rh: new pull request created: #31437

Details

In response to this:

/cherrypick enterprise-4.7

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-cherrypick-robot

@ahardin-rh: new pull request created: #31438

Details

In response to this:

/cherrypick enterprise-4.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants