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.

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

chore: use local files instead of latest from main branch #2269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

christoph-jerolimov
Copy link
Member

Description

Please explain the changes you made here.

Which issue(s) does this PR fix

  • Fixes #?

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

Copy link
Contributor

Copy link
Contributor

github-actions bot commented Feb 5, 2025

This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 21 days.

@github-actions github-actions bot added the Stale label Feb 5, 2025
- type: url
target: https://github.com/redhat-developer/rhdh/blob/main/catalog-entities/all.yaml
- type: file
target: ../../catalog-info.yaml
Copy link

Choose a reason for hiding this comment

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

when this app-config is being parsed, is the CWD in ./packages/app? Just making sure that it's okay to put ../../ in a file that is already on the root repo directory

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, the default is packages/backend, the default Backstage scaffolder contains a comment like this:

https://github.com/christoph-jerolimov/backstage-history/blob/main/app-config.yaml#L80-L95

That was removed here. I think this app-config.yaml is only used in local development. Production installation will not see all these "test entities".

But it's maybe worthful to check that again with someone that is more involved into the build process.

Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

The changes look good and verified working. Just want to understand why we want to use the local file instead of latest main branch file 🤔

- type: file
target: ../../catalog-entities/all.yaml
rules:
- allow: [Component, System, Group, Resource, Location, Template, API]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to allow all entity types here, we can remove the rules right?

@github-actions github-actions bot removed the Stale label Feb 8, 2025
@christoph-jerolimov
Copy link
Member Author

The changes look good and verified working. Just want to understand why we want to use the local file instead of latest main branch file 🤔

@ciiay good question. The reason is: We can't change the e2e tests this way.

When you change an entity and also updates the tests, it still loads the data from the main branch instead of your local copy.

Copy link

@logonoff logonoff left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Feb 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ciiay, logonoff
Once this PR has been reviewed and has the lgtm label, please ask for approval from christoph-jerolimov. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants