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

panlint: Add check for features being included from outside their tree #261

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

Conversation

jrha
Copy link
Member

@jrha jrha commented Oct 24, 2024

i.e. one feature including a template from another

@jrha jrha added the panlint label Oct 24, 2024
Copy link
Contributor

@ned21 ned21 left a comment

Choose a reason for hiding this comment

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

What the goal here? To enforce a rigid hierarchy? That seems a bit too opinionated and is not how we structure our templates, nor can I really think of any good reasons you would want to enforce that.

@jouvin
Copy link
Contributor

jouvin commented Oct 27, 2024

+1. I fully support @ned21 remark. In the template library we maintain, like the grid ou cloud ones (not to speak of our local templates), we have cases where a feature depends on other features. I don't see any reason to forbid it. If you'd like panlint to check for this, I think it should be a panlint option, not the default behaviour.

@jrha
Copy link
Member Author

jrha commented Oct 28, 2024

Fair enough, I'll make it optional. We enforce a two layer hierarchy at RAL which was originally based on discussions with @gombasg I think, where the only includes that are permitted are:

  • From personality/… include feature/…
  • From feature/… include common/…, components/…, (+ other templates in the feature)

@jrha jrha force-pushed the panlint_feature_tree branch 2 times, most recently from ac1c684 to 371c16c Compare December 3, 2024 10:21
@jrha jrha force-pushed the panlint_feature_tree branch 2 times, most recently from 887e564 to 22e0ff7 Compare December 3, 2024 10:28
@jrha jrha requested a review from ned21 December 3, 2024 10:30
@jrha jrha marked this pull request as draft December 3, 2024 10:43
@jrha jrha force-pushed the panlint_feature_tree branch 3 times, most recently from bc5e940 to 338a8ac Compare December 3, 2024 11:36
@jrha jrha added question and removed question labels Dec 3, 2024
@jrha jrha marked this pull request as ready for review December 3, 2024 11:44
Copy link
Contributor

@ned21 ned21 left a comment

Choose a reason for hiding this comment

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

Can you make this optional and off by default please? We don't require this, and in fact actively encourage contrary behaviour because our feature namespace is a mirror of the namespace used by other artefact repositories. Our general policy is that features should be "complete": therefore if a feature depends on another, you MUST include that feature in your feature, and this would break that.

As someone who like a clean and tidy code base, I can appreciate the desire for structure but we find git grep works extremely well within our "monorepo" of templates.

@jrha
Copy link
Member Author

jrha commented Dec 4, 2024

It is optional and off by default.

i.e. they do not include templates from feature paths outside their tree

Whether this is desirable or not is down to site policy, so this is optional and disabled by default.
@jrha
Copy link
Member Author

jrha commented Dec 4, 2024

I have amended the commit message to clarify this.

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

Successfully merging this pull request may close these issues.

3 participants