Skip to content

Conversation

@timflannagan
Copy link
Member

Update internal/declcfg/load.go implementation and avoid processing any files that are named .indexignore when walking the declarative config index root filesystem. When validating declarative config directories, the .indexignore file was being processed and validated.

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1989398.

@openshift-ci openshift-ci bot requested review from estroz and njhale August 3, 2021 16:47
… the root fs

Update internal/declcfg/load.go and avoid processing any files that are
named `.indexignore` when walking the declarative config index root
filesystem. When validating declarative config directories, the
.indexignore file was being processed and validated.

Signed-off-by: timflannagan <timflannagan@gmail.com>
@timflannagan timflannagan force-pushed the update-opm-index-ignore branch from ff73170 to 8383667 Compare August 3, 2021 16:47
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #733 (ff73170) into master (5fc0f42) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head ff73170 differs from pull request most recent head 8383667. Consider uploading reports for the commit 8383667 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
- Coverage   50.05%   50.04%   -0.02%     
==========================================
  Files         100      100              
  Lines        8530     8530              
==========================================
- Hits         4270     4269       -1     
- Misses       3437     3438       +1     
  Partials      823      823              
Impacted Files Coverage Δ
internal/declcfg/load.go 83.56% <100.00%> (ø)
pkg/registry/query.go 61.00% <0.00%> (-0.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fc0f42...8383667. Read the comment docs.

@joelanford
Copy link
Member

This is probably reasonable, but another solution could be to ignore these files using a pattern in the .indexignore file.

The canonical example I use for .indexignore is to explicitly ignore everything except non-k8s-object json/yaml files:

**/*
!**/*.yaml
!**/*.json
**/objects/*
**/objects/*

But I'm good with just hardcoding .indexignore to be ignored since there's no use case to process it as DC.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2021
@joelanford
Copy link
Member

We need a similar change in oc adm catalog mirror here.

@timflannagan
Copy link
Member Author

Yeah, I had the same thought locally and it wasn't immediately clear to me what the best UX is here. I'm under the impression that we should avoid validating the .indexignore file, as I don't think we need to be validating individual patterns outlined in that file, so misconfiguring the .indexignore file would fall under user-error. That said, I think you can make the argument that using a more general pattern (i.e. only validate YAML/JSON files and ignore everything else) is valid, but whether it's the best UX is another question.

@ecordell
Copy link
Member

ecordell commented Aug 3, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, timflannagan

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2021
@openshift-ci openshift-ci bot merged commit 78f27b3 into operator-framework:master Aug 3, 2021
@timflannagan timflannagan deleted the update-opm-index-ignore branch August 3, 2021 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants