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

Remove egress steps not carried out by System Manager #1434

Conversation

edwardchalstrey1
Copy link
Contributor

@edwardchalstrey1 edwardchalstrey1 commented Mar 22, 2023

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the develop branch.
  • Your branch is up-to-date with the develop branch (you probably started your branch from develop but it may have changed since then).
  • If-and-only-if your changes are not yet ready to merge, you have marked this pull request as a draft pull request and added '[WIP]' to the title.
  • If-and-only-if you have changed any Powershell code, you have run the code formatter. You can do this with ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory>.

⤴️ Summary

The second part of this section is instructions for the egress-ee, not the System Manager - I'm removing this to avoid confusion. I will also update Turing internal documentation to have the information that is removed here, making egress documented in the same way as ingress.

🌂 Related issues

Closes #1420

Tandem PR on Turing docs: https://github.com/alan-turing-institute/trusted-research/pull/156

🔬 Tests

cd docs
make html

@JimMadge
Copy link
Member

Hi @edwardchalstrey1,

I think this section of documentation is a bit poor and confusing in general.

  • It isn't clear who receives the outputs. It mentions both 'you' (i.e. the system manager) and the data provider.
  • It doesn't make clear that there should be a review process before creating the egress link!

I have made some changes and pushed them to this branch. Could you add those to your fork's branch and push them so they are part of the PR or allow edits?

@edwardchalstrey1
Copy link
Contributor Author

Screenshot 2023-03-29 at 13 53 29
@JimMadge it looks like you should already be able to edit this PR?

@edwardchalstrey1
Copy link
Contributor Author

@JimMadge I was able to sync your edits

@JimMadge
Copy link
Member

Screenshot 2023-03-29 at 13 53 29 @JimMadge it looks like you should already be able to edit this PR?

Hmm, yes it works. Not sure what I was doing wrong before 🤦

@JimMadge JimMadge requested a review from harisood March 29, 2023 13:10
@JimMadge
Copy link
Member

I'm not worried about the remaining link checking errors. The website exists and the check is probably failing due to rate limiting.

@edwardchalstrey1 and @harisood can you build and read the documentation for a sanity check? I think it is quite poor in the current state (see my comment above).

@edwardchalstrey1
Copy link
Contributor Author

edwardchalstrey1 commented Mar 29, 2023

@JimMadge

  • It isn't clear who receives the outputs. It mentions both 'you' (i.e. the system manager) and the data provider.

Updated

  • It doesn't make clear that there should be a review process before creating the egress link!

Do we want to say this in the DSH docs? I'm thinking that we document our own processes for the Turing prod DSH for this, but for the open source software is it not up to whoever uses it to figure this out?

@JimMadge
Copy link
Member

JimMadge commented Mar 29, 2023

Thanks @edwardchalstrey1 I missed that reference.

  • It doesn't make clear that there should be a review process before creating the egress link!

Do we want to say this in the DSH docs? I'm thinking that we document our own processes for the Turing prod DSH for this, but for the open source software is it not up to whoever uses it to figure this out?

I'm pretty certain that we do. It isn't a discussion of the Turing's (or any organisations) particular process. It is just stating that outputs need to be reviewed. I don't think you can have a functional TRE if you can remove data without review.

This is also consistent with how the data ingress steps are described.

@edwardchalstrey1
Copy link
Contributor Author

Do we want to say this in the DSH docs? I'm thinking that we document our own processes for the Turing prod DSH for this, but for the open source software is it not up to whoever uses it to figure this out?
I'm pretty certain that we do. It isn't a discussion of the Turing's (or any organisations) particular process. It is just stating that outputs need to be reviewed. I don't think you can have a functional TRE if you can remove data without review.

Agreed, just realised you have added this already

@JimMadge I'm happy with this now, but let me know if you think further edits needed

Copy link
Contributor

@craddm craddm left a comment

Choose a reason for hiding this comment

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

LGTM

@edwardchalstrey1 edwardchalstrey1 merged commit ea0e0da into alan-turing-institute:develop Apr 5, 2023
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.

Move egress download doc section from System Manager doc somewhere else
3 participants