Skip to content

Conversation

@aarnq
Copy link
Contributor

@aarnq aarnq commented Nov 27, 2024

Warning

This is a public repository, ensure not to disclose:

  • personal data beyond what is necessary for interacting with this pull request, nor
  • business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

  • kind/feature
  • kind/improvement
  • kind/deprecation
  • kind/documentation
  • kind/clean-up
  • kind/bug
  • kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

  • kind/admin-change
  • kind/dev-change
  • kind/security
  • kind/adr

What does this PR do / why do we need this PR?

Updates issue templates, removes the release template, and updates the linting based on changes done in the docs repo.

Information to reviewers

The wildest change is to rename all go-templates bases and stacks, but that will be come a requirement in helmfile (once they decide to release v1), and we already use that for our values templates.
And it fits more naturally, because they aren't yaml.

And as it touches a bit of everything, I believe everyone is a reviewer, sorry bout that 😅

Checklist

  • Proper commit message prefix on all commits
  • Change checks:
    • The change is transparent
    • The change is disruptive
    • The change requires no migration steps
    • The change requires migration steps
    • The change upgrades CRDs
    • The change updates the config and the schema
  • Documentation checks:
  • Metrics checks:
    • The metrics are still exposed and present in Grafana after the change
    • The metrics names didn't change (Grafana dashboards and Prometheus alerts are not affected)
    • The metrics names did change (Grafana dashboards and Prometheus alerts were fixed)
  • Logs checks:
    • The logs do not show any errors after the change
  • Pod Security Policy checks:
    • Any changed pod is covered by Pod Security Admission
    • Any changed pod is covered by Gatekeeper Pod Security Policies
    • The change does not cause any pods to be blocked by Pod Security Admission or Policies
  • Network Policy checks:
    • Any changed pod is covered by Network Policies
    • The change does not cause any dropped packets in the NetworkPolicy Dashboard
  • Audit checks:
    • The change does not cause any unnecessary Kubernetes audit events
    • The change requires changes to Kubernetes audit policy
  • Falco checks:
    • The change does not cause any alerts to be generated by Falco
  • Bug checks:
    • The bug fix is covered by regression tests

@aarnq aarnq self-assigned this Nov 27, 2024
@aarnq aarnq added QA Issues regarding tests and QA kind/documentation Improvements or additions to documentation labels Nov 27, 2024
@aarnq aarnq force-pushed the aarnq/update-templates branch from 04953f7 to 09f0cbf Compare December 5, 2024 09:57
@aarnq aarnq force-pushed the aarnq/update-templates branch from 09f0cbf to f2a35d1 Compare December 17, 2024 07:55
@aarnq aarnq marked this pull request as ready for review December 17, 2024 08:03
@aarnq aarnq requested review from a team as code owners December 17, 2024 08:03
@aarnq aarnq requested review from a team December 17, 2024 08:03
@aarnq aarnq requested review from a team as code owners December 17, 2024 08:03
Copy link
Contributor

@AlbinB97 AlbinB97 left a comment

Choose a reason for hiding this comment

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

This looks great, amazing work!

Copy link
Contributor

@anders-elastisys anders-elastisys left a comment

Choose a reason for hiding this comment

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

LGTM, just had one comment

Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

I need to understand the reason why the QA checklist is made private. We really like to show this and it's even linked to from our public docs.

@aarnq
Copy link
Contributor Author

aarnq commented Dec 18, 2024

I need to understand the reason why the QA checklist is made private. We really like to show this and it's even linked to from our public docs.

Because the process simply hasn't worked well after the split, so the checklist that we need for our process must be together with the internal one.

I do also want to expose it in some way to make the actual QA part public, but I haven't figured out a good way to do it yet. My thinking is a document in each artefact repository, as then it will also serve as a reference for contributors for what we expect to be tested.
However it would be formatted essentially as "this is what we test, and what we expect to be tested" document, rather than the unwieldy checklist itself.

@aarnq
Copy link
Contributor Author

aarnq commented Dec 18, 2024

I need to understand the reason why the QA checklist is made private. We really like to show this and it's even linked to from our public docs.

Because the process simply hasn't worked well after the split, so the checklist that we need for our process must be together with the internal one.

I do also want to expose it in some way to make the actual QA part public, but I haven't figured out a good way to do it yet. My thinking is a document in each artefact repository, as then it will also serve as a reference for contributors for what we expect to be tested. However it would be formatted essentially as "this is what we test, and what we expect to be tested" document, rather than the unwieldy checklist itself.

Restored and updated it with the current changes, along with an alert that it is going to be replaced.

Copy link
Contributor

@viktor-f viktor-f left a comment

Choose a reason for hiding this comment

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

A lot of nice cleanup, well done.

@aarnq aarnq requested review from a team as code owners December 18, 2024 15:35
Copy link
Contributor

@viktor-f viktor-f left a comment

Choose a reason for hiding this comment

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

Nice work with these updates, I think it looks good.

One small nit for future PRs: I think that these changes could actually have been a few different commits (possibly different PRs), since it to me feels like there are several different things happening here that does not necessarily belong together. E.g. it could have been split to one for updating templates, one for lint changes, one for renaming the stacks files. Another alternative would be to have a longer commit message that makes it more clear the different things that were included, or something that just makes it clear that there are several different things in it without specifying exactly what.
I do not think that it is worth reworking this now and I do not see it as important. But if you agree with my reasoning, then keep it in mind for future PRs. 🙂

@aarnq
Copy link
Contributor Author

aarnq commented Dec 19, 2024

Nice work with these updates, I think it looks good.

One small nit for future PRs: I think that these changes could actually have been a few different commits (possibly different PRs), since it to me feels like there are several different things happening here that does not necessarily belong together. E.g. it could have been split to one for updating templates, one for lint changes, one for renaming the stacks files. Another alternative would be to have a longer commit message that makes it more clear the different things that were included, or something that just makes it clear that there are several different things in it without specifying exactly what. I do not think that it is worth reworking this now and I do not see it as important. But if you agree with my reasoning, then keep it in mind for future PRs. 🙂

Thanks, I'll think about that!

The problem is that the "templates" that I'm producing consist of pre-commit hooks and configs, workflows, issue and pull request templates, even some scripts. 😅
But I will definitely split some of them up onto separate commits so it is clearer from the start what originates from where.

Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

Thanks for keeping our quality high! Let's discuss how to make the quality checklist public once this PR is merged and the linkchecker on elastisys.io complains. 😄

Including issue and pull request templates, worflows, pre-commit
configs, some scripts and documentation.

This also deprecates the release issue found in this repository, as it
has been reunified into the internal release issue.
The deprecated one will be kept for reference until a better document is
provided.
@aarnq aarnq force-pushed the aarnq/update-templates branch from c60d69a to 98331f2 Compare December 20, 2024 07:33
@aarnq aarnq merged commit 98331f2 into main Dec 20, 2024
12 checks passed
@aarnq aarnq deleted the aarnq/update-templates branch December 20, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/documentation Improvements or additions to documentation QA Issues regarding tests and QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants