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

add qa script and readme #1388

Draft
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

SimoneDutto
Copy link
Contributor

Description

While qa locally ReBAC Admin I made some change to the original rebac-admin-ui-handlers script to adapt it to jimm's deployment.
After discussing with @babakks we decided to add it to Jimm's repo for reproducibility.

Fixes JIRA/GitHub issue number

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

Notes for code reviewers

@SimoneDutto SimoneDutto requested a review from a team as a code owner October 9, 2024 10:35
babakks
babakks previously approved these changes Oct 9, 2024
kian99
kian99 previously approved these changes Oct 9, 2024
Copy link
Contributor

@kian99 kian99 left a comment

Choose a reason for hiding this comment

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

To understand better, does testing like this offer us anything beyond what our integration tests do?

local/rebac-admin/qa.sh Outdated Show resolved Hide resolved

To QA locally the ReBAC Admin Backend you need to:

## Change the rebac authentication middleware at `internal/middleware/authn.go` to:
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit flimsy because the code below will not stay up to date as the actual code changes.
Also, we don't mention what the change is doing?
Could we not run the tests with the default admin user in the docker compose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is the easiest way to remove the authentication but keep the user in the context for the rebac-admin stuff to work.
Otherwise we need to create a cookie and use it in the bash script, or something like taht

@SimoneDutto
Copy link
Contributor Author

@kian99 to answer you: it's not adding much to the whole integration tests. In fact it is not run in a action, it is just because @babakks mentioned that this script could be useful so he suggested to add to Jimm's repo

@SimoneDutto SimoneDutto dismissed stale reviews from kian99 and babakks via ac170d6 October 9, 2024 13:14
@kian99
Copy link
Contributor

kian99 commented Oct 10, 2024

@kian99 to answer you: it's not adding much to the whole integration tests. In fact it is not run in a action, it is just because @babakks mentioned that this script could be useful so he suggested to add to Jimm's repo

In that case I'd question whether we should put it in the repo with the additional instructions on having to modify the source code to get it to run. It feels like "dead code" something that won't get used and potentially only cause confusion to someone that looks at it in a few years time. Maybe instead we could include in the readme a link to the test script in the other repo, etc.

@SimoneDutto
Copy link
Contributor Author

@kian99 to answer you: it's not adding much to the whole integration tests. In fact it is not run in a action, it is just because @babakks mentioned that this script could be useful so he suggested to add to Jimm's repo

In that case I'd question whether we should put it in the repo with the additional instructions on having to modify the source code to get it to run. It feels like "dead code" something that won't get used and potentially only cause confusion to someone that looks at it in a few years time. Maybe instead we could include in the readme a link to the test script in the other repo, etc.

Since it is temporary until we have a proper QA from the Web Team we can even leave this PR open for reference and close the PR as soon as the QA have been done.

@babakks what do you think?

@SimoneDutto SimoneDutto marked this pull request as draft October 18, 2024 12:04
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.

3 participants