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

fix: lf footer website guidelines #1678

Merged
merged 13 commits into from
Sep 3, 2024
Merged

fix: lf footer website guidelines #1678

merged 13 commits into from
Sep 3, 2024

Conversation

degenaro
Copy link
Collaborator

https://github.com/cncf/foundation/blob/main/website-guidelines.md

Types of changes

  • Hot fix (emergency fix and release)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation (change which affects the documentation site)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Release (develop -> main)

Quality assurance (all should be covered).

  • My code follows the code style of this project.
  • Documentation for my change is up to date?
  • My PR meets testing requirements.
  • All new and existing tests passed.
  • All commits are signed-off.

Summary

Key links:

Before you merge

  • Ensure it is a 'squash commit' if not a release.
  • Ensure CI is currently passing
  • Check sonar. If you are working for a fork a maintainer will reach out, if required.

@degenaro degenaro changed the title LF footer website guidelines fix: LF footer website guidelines Aug 30, 2024
@degenaro degenaro changed the title fix: LF footer website guidelines fix: lf footer website guidelines Aug 30, 2024
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
Copy link
Member

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

Is the goal here to update the compliance-trestle website as well or just the README? The website is not changed by because there is a copy of the README here https://github.com/oscal-compass/compliance-trestle/blob/develop/docs/index.md

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
Signed-off-by: Lou DeGenaro <[email protected]>
@degenaro
Copy link
Collaborator Author

Is the goal here to update the compliance-trestle website as well or just the README? The website is not changed by because there is a copy of the README here https://github.com/oscal-compass/compliance-trestle/blob/develop/docs/index.md

Fair point. This should be updated as well. Kinda ugly to have essentially 2 copies.

Signed-off-by: Lou DeGenaro <[email protected]>
@degenaro
Copy link
Collaborator Author

@jpower432 Thx for your careful review. My claim is that all these problems have been remedied.

@@ -112,7 +112,7 @@ If you would like to see the detailed LICENSE click [here](LICENSE).
Consult [contributors](https://github.com/oscal-compass/compliance-trestle/graphs/contributors) for a list of authors and [maintainers](MAINTAINERS.md) for the core team.

```text
# Copyright (c) 2020 IBM Corp. All rights reserved.
# Copyright (c) 2024 The OSCAL Compass Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the story on the headers in the python files?

Do we need to replace those as well? @jpower432

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've seen nothing the in the CNCF read and understand documentation for such a requirement. Just about acceptable licenses, which we have already.

Copy link
Member

Choose a reason for hiding this comment

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

Here is some CNCF guidance I found around copyright notices. I do think updating existing notices is something we should complete based this guidance and CNCF project convention, but this could be considered out of scope for this PR IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The guidance clearly states: Please note that it is not wrong, and it is acceptable, if a contributor wishes to keep their own copyright notices on their contributions. The above is a recommended format for ease of use, but is not mandated by CNCF. My view is that we should leave the existing notices in place, and any new modules will get the new notice. However, it the majority feel uniformity is desirable I will not be a blocker.

Copy link
Member

Choose a reason for hiding this comment

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

@degenaro I understand. I did not mean to imply this was a requirement. I read it as a recommendation. In any case, it seems we would need prior approval per this section of the guidance - You should not change or remove someone else's copyright notice unless they have expressly permitted you to do so. This includes third parties' notices in pre-existing code.

@@ -1 +1,13 @@
{!MAINTAINERS.md!}
Trestle was designed and open sourced by a team based at [IBM Research](https://www.research.ibm.com/) and others around the world. The list includes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be changed. The mainters reference automatically reads the file at the root of the directory of the repository. That file should be changed instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@butler54
Copy link
Collaborator

fair point. This should be updated as well. Kinda ugly to have essentially 2 copies.

I think in this context @degenaro we can de-duplicate by pulling in external markdown files. For the index file it's a little tricky as we might make relative link assumptions. I've updated a website epic #1677 with this.

@butler54
Copy link
Collaborator

Another question do we need to add this as a footer to all pages. It's a pretty easy config: https://squidfunk.github.io/mkdocs-material/setup/setting-up-the-footer/?h=footer#copyright-notice (maybe no the logo on every page).

Signed-off-by: Lou DeGenaro <[email protected]>
@degenaro
Copy link
Collaborator Author

degenaro commented Sep 2, 2024

fair point. This should be updated as well. Kinda ugly to have essentially 2 copies.

I think in this context @degenaro we can de-duplicate by pulling in external markdown files. For the index file it's a little tricky as we might make relative link assumptions. I've updated a website epic #1677 with this.

No need to fix now. Future fix in website epic is good idea.

@degenaro
Copy link
Collaborator Author

degenaro commented Sep 2, 2024

Another question do we need to add this as a footer to all pages. It's a pretty easy config: https://squidfunk.github.io/mkdocs-material/setup/setting-up-the-footer/?h=footer#copyright-notice (maybe no the logo on every page).

The guidelines https://github.com/cncf/foundation/blob/main/website-guidelines.md are not crystal clear. I think that if we had a footer on every page, then we'd need to update it. But using https://kyverno.io/ as a model they just have a footer on the homepage. I don't think we are required to add footers on every page.

Copy link
Member

@jflowers jflowers left a comment

Choose a reason for hiding this comment

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

Please remove Red Hat from the footer

Copy link
Member

@jflowers jflowers left a comment

Choose a reason for hiding this comment

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

abstain

Copy link
Collaborator

@butler54 butler54 left a comment

Choose a reason for hiding this comment

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

LGTM

@degenaro degenaro merged commit 40b2880 into develop Sep 3, 2024
14 checks passed
@degenaro degenaro deleted the LF-website-guidelines branch September 3, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants