Complete documentation coverage of registry operator #46

feloy · 2023-08-25T09:35:24Z

Please specify the area for this PR

What does does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes devfile/api#1015
Fixes partially devfile/api#1233

PR acceptance criteria:

Test Coverage
- Are your changes sufficiently tested, and are any applicable test cases added or updated to cover your changes?
Gosec scans
- Fix all MEDIUM and higher findings and/or annotate a finding per gosec instructions: https://github.com/securego/gosec#annotating-code to address why a finding is not a security issue

Documentation

Does the registry operator documentation need to updated with your changes?

How to test changes / Special notes to the reviewer:

codecov · 2023-08-25T12:01:14Z

Codecov Report

Patch coverage has no change and project coverage change: +0.61% 🎉

Comparison is base (dea070a) 22.11% compared to head (8f28abc) 22.72%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   22.11%   22.72%   +0.61%     
==========================================
  Files          23       23              
  Lines        1307     1307              
==========================================
+ Hits          289      297       +8     
+ Misses       1001      995       -6     
+ Partials       17       15       -2

Files Changed	Coverage Δ
api/v1alpha1/devfileregistry_types.go	`100.00% <ø> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

feloy · 2023-08-28T10:09:29Z

DEVFILE_REGISTRY.md

+    registryName: test
+  storage:
+    enabled: true
+    ociRegistryImage: 2Gi


This should be registryVolumeSize IMO, but the code currently indicate this:

RegistryVolumeSize string `json:"ociRegistryImage,omitempty"`

Do we want to change the code directly, or do we want to deprecate it?

Preferably we should fix it before documenting it as it's a typo.

feloy · 2023-08-28T10:10:07Z

DEVFILE_REGISTRY.md

+    registryName: test
+  tls:
+    enabled: true
+    ociRegistryImage: my-tls-secret


This should be secretName IMO, but the code currently indicates this:

SecretName string `json:"ociRegistryImage,omitempty"`

Do we want to change the code directly, or do we want to deprecate it?

Preferably we should fix it before documenting it as it's a typo.

Signed-off-by: Philippe Martin <[email protected]>

…ot used Signed-off-by: Philippe Martin <[email protected]>

Signed-off-by: Philippe Martin <[email protected]>

johnmcollier · 2023-08-28T16:34:33Z

DEVFILE_REGISTRY.md

+EOF
+```
+
+## Using a Persistent Volume to store OCI images


As part of devfile/api#1093, we will be retiring the storage field for the devfile registry, as it's no longer used. So we should leave it out of the documentation

johnmcollier · 2023-08-28T16:35:59Z

DEVFILE_REGISTRY.md

+    registryName: test
+  tls:
+    enabled: true
+    ociRegistryImage: my-tls-secret


Preferably we should fix it before documenting it as it's a typo.

johnmcollier · 2023-08-28T16:36:17Z

DEVFILE_REGISTRY.md

+    registryName: test
+  storage:
+    enabled: true
+    ociRegistryImage: 2Gi


Preferably we should fix it before documenting it as it's a typo.

Signed-off-by: Philippe Martin <[email protected]>

michael-valdron

Looks great! I add one feedback comment on the first time setup.

Additionally, we'll need to run make install-cert before running make install && make deploy

michael-valdron · 2023-08-29T23:07:41Z

CONTRIBUTING.md

 #### First Time Setup
 1. Install prerequisites:
-   - Go 1.13 or higher
+   - Go 1.18 or higher
   - Docker or Podman
-   - Operator-SDK 1.11.0 or higher (including `controller-gen` 0.6.0 or higher)
+   - Operator-SDK 1.28.0 or higher (including `controller-gen` 0.10.0 or higher)


We could link to README Requirements instead or vice versa, as the content in these places are the same.

Signed-off-by: Philippe Martin <[email protected]>

feloy · 2023-08-30T07:31:31Z

Looks great! I add one feedback comment on the first time setup.

Additionally, we'll need to run make install-cert before running make install && make deploy

@michael-valdron Thanks for your review. I have pushed a new commit for first 2 points of devfile/api#1233

I don't understand what the third one means (I cannot find such a command in Makefile):

 Include docker-buildx as an optional build and push step for multi arch

michael-valdron · 2023-08-30T14:51:13Z

README.md


 To see all rules supported by the makefile, run `make help`

 ## Testing

 To run integration tests for the operator, run `make test-integration`. 

-By default, the tests will use the default image for the operator, `quay.io/devfile/registry-operator:next`. To use your own image, run:
+The `oc` executable must be accessible.


In the current state this is required, however we will need to open an issue to allow kubectl to be used for the integration test suite given we support OpenShift and Kubernetes.

michael-valdron

/lgtm

Looks great! I add one feedback comment on the first time setup.
Additionally, we'll need to run make install-cert before running make install && make deploy

@michael-valdron Thanks for your review. I have pushed a new commit for first 2 points of devfile/api#1233

I don't understand what the third one means (I cannot find such a command in Makefile):
 Include docker-buildx as an optional build and push step for multi arch

This is a new rule that will be added in #45, I can include this doc change there.

openshift-ci · 2023-08-30T14:52:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, michael-valdron

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~~ [michael-valdron]

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

michael-valdron · 2023-08-30T14:53:08Z

/retest

* Add more commands in README Signed-off-by: Philippe Martin <[email protected]> * Remove definition of NAMESPACE when executing make run, which seems not used Signed-off-by: Philippe Martin <[email protected]> * Update CONTRIBUTING Signed-off-by: Philippe Martin <[email protected]> * Document DevfileRegistry resource Signed-off-by: Philippe Martin <[email protected]> * Update REGISTRIES_LISTS Signed-off-by: Philippe Martin <[email protected]> * Fix typo "ociRegistryImage" instead of registryVolumeSize and secretName Signed-off-by: Philippe Martin <[email protected]> * No doc about Storage Signed-off-by: Philippe Martin <[email protected]> * Update CONTRIBUTING Signed-off-by: Philippe Martin <[email protected]> --------- Signed-off-by: Philippe Martin <[email protected]> Signed-off-by: thepetk <[email protected]>

openshift-ci bot requested review from elsony and mike-hoang August 25, 2023 09:35

feloy marked this pull request as draft August 25, 2023 09:35

openshift-ci bot added the do-not-merge/work-in-progress label Aug 25, 2023

feloy force-pushed the doc-1015 branch from 10f6038 to fffda46 Compare August 25, 2023 11:55

feloy force-pushed the doc-1015 branch from 445d40e to de6074c Compare August 28, 2023 10:07

feloy commented Aug 28, 2023

View reviewed changes

feloy added 5 commits August 28, 2023 12:23

Add more commands in README

1df0389

Signed-off-by: Philippe Martin <[email protected]>

Remove definition of NAMESPACE when executing make run, which seems n…

058f593

…ot used Signed-off-by: Philippe Martin <[email protected]>

Update CONTRIBUTING

66726e8

Signed-off-by: Philippe Martin <[email protected]>

Document DevfileRegistry resource

8ccfaa0

Signed-off-by: Philippe Martin <[email protected]>

Update REGISTRIES_LISTS

b82ab9d

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the doc-1015 branch from de6074c to b82ab9d Compare August 28, 2023 10:24

feloy marked this pull request as ready for review August 28, 2023 10:24

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 28, 2023

openshift-ci bot requested review from johnmcollier and kim-tsao August 28, 2023 10:24

johnmcollier reviewed Aug 28, 2023

View reviewed changes

feloy added 2 commits August 29, 2023 13:25

Fix typo "ociRegistryImage" instead of registryVolumeSize and secretName

6283507

Signed-off-by: Philippe Martin <[email protected]>

No doc about Storage

8f28abc

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the doc-1015 branch from 18811d6 to 8f28abc Compare August 29, 2023 11:25

feloy requested a review from johnmcollier August 29, 2023 11:25

michael-valdron reviewed Aug 29, 2023

View reviewed changes

Update CONTRIBUTING

e71d08a

Signed-off-by: Philippe Martin <[email protected]>

feloy requested a review from michael-valdron August 30, 2023 07:31

michael-valdron reviewed Aug 30, 2023

View reviewed changes

michael-valdron approved these changes Aug 30, 2023

View reviewed changes

openshift-ci bot assigned michael-valdron Aug 30, 2023

openshift-ci bot added the lgtm label Aug 30, 2023

openshift-ci bot added the approved label Aug 30, 2023

michael-valdron merged commit 9ac9961 into devfile:main Aug 30, 2023
5 checks passed

This was referenced Aug 30, 2023

Update CONTRIBUTING for registry operator devfile/api#1233

Closed

Devfile registry operator pre-release to OperatorHub devfile/api#1211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete documentation coverage of registry operator #46

Complete documentation coverage of registry operator #46

feloy commented Aug 25, 2023 •

edited by michael-valdron

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading

feloy Aug 28, 2023

johnmcollier Aug 28, 2023

feloy Aug 28, 2023

johnmcollier Aug 28, 2023

johnmcollier Aug 28, 2023

johnmcollier Aug 28, 2023

johnmcollier Aug 28, 2023

michael-valdron left a comment •

edited

Loading

michael-valdron Aug 29, 2023

feloy commented Aug 30, 2023 •

edited

Loading

michael-valdron Aug 30, 2023

michael-valdron left a comment •

edited

Loading

openshift-ci bot commented Aug 30, 2023

michael-valdron commented Aug 30, 2023

Complete documentation coverage of registry operator #46

Complete documentation coverage of registry operator #46

Conversation

feloy commented Aug 25, 2023 • edited by michael-valdron Loading

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-valdron left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feloy commented Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

michael-valdron left a comment • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Aug 30, 2023

michael-valdron commented Aug 30, 2023

feloy commented Aug 25, 2023 •

edited by michael-valdron

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading

michael-valdron left a comment •

edited

Loading

feloy commented Aug 30, 2023 •

edited

Loading

michael-valdron left a comment •

edited

Loading