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

Point to Airlock for viewing log outputs #4585

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

evansd
Copy link
Contributor

@evansd evansd commented Sep 3, 2024

There's no point linking directly to the relevant Airlock URL because we don't expect users to be accessing Job Server from within Level 4 going forward, and even you have Airlock open in a VM you can't copy/paste URLs across.

Instead we link to the Airlock docs and describe the location of the file. This replaces both the link the output viewer and the reference to the old "Level 4 file sync" directory.

Example:
image

Previously we restricted these links to only show for the TPP backend, but on the assumption that we will be running an instance of Airlock for every backend I have removed this restriction.

There's now a significant chunk of redundant code in Job Server relating to the "Level 4 files viewer". I'll make a separate ticket to track what needs removing here, and will tackle this in future PRs.

Closes #4583

@iaindillingham
Copy link
Member

Does RAP plan to link to Airlock from the WorkspaceDetail view? I wondered whether this PR made Workspace.get_files_url unnecessary. It doesn't, but discovered that we link to the output viewer in templates/workspace/detail.html.

@rebkwok
Copy link
Contributor

rebkwok commented Sep 4, 2024

We have mentioned in the past that we should remove the option to view L4 outputs from job-server - this is the reference to get_files_url that @iaindillingham pointed out.
https://github.com/opensafely-core/job-server/blob/main/templates/workspace/detail.html#L259C1-L262C32

I think we can and should do that now, as we've now updated the docs to remove reference to viewing L4 files via job server. There should now be no reason for users to log into job-server on L4 (and theoretically we'll remove access to it at some point). Of course, that does mean that linking to the log file in Airlock from job-server is a bit pointless (or will be, if users aren't logging into it). Should we instead link to the docs on viewing outputs instead, with a note to find the file at metadata/<log file>.log in the workspace?

I also opened this new ticket based on Simon's comment on the previous docs PR, and the extra "viewing output files" section could be a place to describe how to find log files in Airlock.

@bloodearnest
Copy link
Member

I think direct link to Airlock from job-server is possibly confusing.

It will resolve to the DNS/SSL holding page we use to generate certificates: https://tpp.backends.opensafely.org/

We are about to rip out the level 4 login, at which point, you can't even log in to job-server from level 4 to view this link in a context where it would correctly resolve to airlock.

Maybe we should instead link to the "Logging in to airlock" docs?

There's no point linking directly to the relevant Airlock URL because we
don't expect users to be accessing Job Server from within Level 4 going
forward, and even you have Airlock open in a VM you can't copy/paste
URLs across.

Instead we link to the Airlock docs and describe the location of the
file. This replaces both the link the output viewer and the reference to
the old "Level 4 file sync" directory.

Previously we restricted these links to only show for the TPP backend,
but on the assumption that we will be running an instance of Airlock for
every backend I have removed this restriction.
@evansd evansd force-pushed the evansd/replace-filesync-with-airlock branch from b8d4cc8 to 438b49c Compare September 4, 2024 15:45
@evansd evansd changed the title Link to Airlock for viewing log outputs Point to Airlock for viewing log outputs Sep 4, 2024
@evansd evansd merged commit a215c45 into main Sep 4, 2024
8 checks passed
@evansd evansd deleted the evansd/replace-filesync-with-airlock branch September 4, 2024 16: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.

Change jobserver failure message to point to airlock not file sync
4 participants