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

interfaces/builtin: allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme #13130

Merged
merged 3 commits into from Mar 14, 2024

Conversation

nteodosio
Copy link
Contributor

LP:2032992 reports that documentation cannot be read in browsers because of the lack of allowance for /usr/share/javascript/jquery/.

I opened a discussion at the Snapcraft forum and was suggested this interface.

Is it the proper one? If yes I'll extend the test file accordingly.

@pedronis pedronis added the Needs security review Can only be merged once security gave a :+1: label Aug 28, 2023
@pedronis pedronis added this to the 2.61 milestone Sep 7, 2023
@mvo5 mvo5 modified the milestones: 2.61, 2.62 Oct 4, 2023
Copy link
Collaborator

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

LGTM.

@alexmurray alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Oct 19, 2023
@nteodosio nteodosio changed the title Allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme. interfaces/system-packages-doc: Allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme. Oct 19, 2023
@nteodosio
Copy link
Contributor Author

Thanks for the review. I included extended the test file as promised.

The automated tests check the pull request title, is that really important? I'd expect it to not go beyond the commit title. Anyway, I edited it, but you'll need to manually trigger the test again apparently.

@lissyx
Copy link
Contributor

lissyx commented Jan 9, 2024

What is missing to land this @nteodosio ?

@nteodosio
Copy link
Contributor Author

I think one more person needs to approve it, judging by the fragment in the reviewers section:

"At least 2 approving reviews are required to merge this pull request."

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

While the new mount entries look relatively harmless I think there's going to be a point where we need to re-think how such documentation is accessed.

It cannot be extended indefinitely without causing problems to what is in the mount namespace.

@lissyx
Copy link
Contributor

lissyx commented Mar 6, 2024

While the new mount entries look relatively harmless I think there's going to be a point where we need to re-think how such documentation is accessed.

It cannot be extended indefinitely without causing problems to what is in the mount namespace.

Agreed, I think @seb128 started something around that but I cannot find the link anymore. I regularly run into bugs filed on bugzilla because of this :(

@lissyx
Copy link
Contributor

lissyx commented Mar 6, 2024

@Meulengracht
Copy link
Member

@nteodosio please rebase on top of master and resolve conflicts

@zyga let's talk about this at the upcoming sprint

@Meulengracht
Copy link
Member

@nteodosio the unit tests are failing, please handle those

@ernestl ernestl self-requested a review March 14, 2024 12:41
@ernestl ernestl changed the title interfaces/system-packages-doc: Allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme. interfaces/builtin: Allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme. Mar 14, 2024
@ernestl ernestl changed the title interfaces/builtin: Allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme. interfaces/builtin: allow access to /usr/share/javascript/{sphinxdoc,jquery} and /usr/share/sphinx_rtd_theme Mar 14, 2024
@ernestl
Copy link
Collaborator

ernestl commented Mar 14, 2024

Checked with Nathan, this is low priority and will not be addressed in time for 2.62, moving to 2.63

@ernestl ernestl modified the milestones: 2.62, 2.63 Mar 14, 2024
@mkaply
Copy link

mkaply commented Mar 14, 2024

Can I ask why you consider this low priority? Not being able to read the docs in browsers is kind of important. There's a patch ready to go here.

@olivercalder
Copy link
Member

I've rebased on master to pull in the latest changes, and I added the missing tests for the mount spec.

@olivercalder
Copy link
Member

Pending spread test passage, this should be good to ship with 2.62.

@nteodosio
Copy link
Contributor Author

Can I ask why you consider this low priority? Not being able to read the docs in browsers is kind of important.

I meant there were higher priority items on my list and that I'd look into it today or tomorrow. Thanks Oliver for taking care of it!

@olivercalder olivercalder modified the milestones: 2.63, 2.62 Mar 14, 2024
@olivercalder olivercalder added the Squash-merge Please squash this PR when merging. label Mar 14, 2024
Copy link
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM

@Meulengracht Meulengracht merged commit 5a8ab2d into snapcore:master Mar 14, 2024
41 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Squash-merge Please squash this PR when merging.
Projects
None yet
10 participants