Support for pages authentication switches
What does this MR do?
This merge request adds support for GitLab pages authentication switches implemented by @tuomoa in gitlab-pages!94 (merged).
Checklist
See Definition of done.
-
Change added to CHANGELOG.md. Not applicable for Documentation changes and minor changes. -
Documentation created/updated -
Tests added -
Integration tests added to GitLab QA, if applicable -
MR targeting master
branch -
MR has a green pipeline on GitLab.com
Reviewer Checklist
In addition to above, reviewer must:
-
Pipeline is green on dev.gitlab.org if the change is not touching documentation or internal cookbooks
References
https://gitlab.com/gitlab-org/gitlab-ce/issues/33422
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18589
Merge request reports
Activity
mentioned in merge request gitlab-pages!94 (merged)
- Resolved by Nick Thomas
@nick.thomas I think it's only missing the
access_control
hint that I added in the GitLab but other than that it should be okay. It might be a good idea to test it also, as I'm not sure how to test it.@Turmio Thanks for the merge request.
A few quick notes:
-
It looks like this depends on gitlab-pages!94 (merged) which isn't yet merged. I'll mark this as blocked for now.
-
You will also want to enable shared runners as per our contributing guide so our automated tests can run.
-
I don't think
@bjk-gitlab
is necessary for an approval on this, it should be safe to remove him from the approvers list.
-
added workflowblocked label
@ibaum thanks! I don't know how I managed to skip whole contributing guide.
added 254 commits
-
a8861481...1bfd77c2 - 252 commits from branch
gitlab-org:master
- c3992dc3 - Support for pages authentication switches
- 653a4215 - Added access_control switch for pages and set it enabled by default
-
a8861481...1bfd77c2 - 252 commits from branch
I added
access_control
setting (defaults to true) and rebased to latest master.@nick.thomas I did not have time to create VM and test omnibus after @ibaum pointed me to right direction. I may have time to read up on testing guide and find out how to use @tuomoa's forks tomorrow but I cannot promise that. At least pipeline went to green.
If everything works fine and secret can be generated as in this MR, I think this is ready.
changed milestone to %11.3
added 1 commit
- 71e65bc3 - Fixed usage of access control properties, previous implementation used always defaults.
Hi @Turmio,
Your latest commits are causing some valid spec failures. Can you take a look?
Hi @ibaum,
Yeah, I will. I just did not have time to fix those when I made changes with Web IDE and thought that I have time later that day. I will fix those failures later this week and rebase to latest master.
added 1 commit
- 50106192 - Fix specs to take account of new subdomain for Pages
added 175 commits
-
50106192...f53c3e7f - 173 commits from branch
gitlab-org:master
- d3424c83 - Merge branch 'master' into pages_authentication
- f73ad3d6 - Some omnibus improvements
-
50106192...f53c3e7f - 173 commits from branch
@Turmio @ibaum @tuomoa I've taken the liberty of pushing some improvements to this MR, but I don't think it's ready yet.
The failing test was easy to fix. What remains, I think, is fettling the gitlab.yml file. Like
gitlab_rails['pages_enabled']
, we need to correctly setgitlab_rails['pages_access_control']
based on thegitlab_pages['access_control']
attribute, and add specs to ensure it does the right thing.I'm happy with the feature itself, so I think I'm going to merge the pages and CE MRs for 11.4 anyway. Without omnibus support, it will be disabled by default. If we can get the omnibus MR in for 11.4 (I'm afraid I can't spend any more time on it myself), great. Otherwise, we can target it for 11.5 to be enabled by default.
mentioned in commit gitlab-pages@86ad3207
@ibaum some of the specs are now failing with:
RuntimeError ------------ The sidekiq_cluster queue_groups must be set in order to use the sidekiq-cluster service
Presumably this is from a change in master that came in with the merge to latest?
added Community contribution devopsverify labels and removed workflowblocked label
changed milestone to %11.4
@nick.thomas sounds reasonable. What is the timeline for 11.4? Maybe I or @Turmio could spend some time on this to get it finished and into the 11.4. :)
- Resolved by Ian Baum
- Resolved by Turo Soisenniemi
- Resolved by Turo Soisenniemi
- Resolved by Turo Soisenniemi
some of the specs are now failing with:
RuntimeError ------------ The sidekiq_cluster queue_groups must be set in order to use the sidekiq-cluster service
@nick.thomas Huh. That is weird. That isn't the reason why tests fail. Please check the JUnit report on this MR to see what actually failed.
I will open an issue to fix the test reports. EDIT: #3831 (closed)
Edited by Balasankar 'Balu' CNote that I doubt we can enable this feature on GitLab.com / gitlab.io without this MR. So we should keep https://gitlab.com/gitlab-org/gitlab-ce/issues/33422 open until it's finished.
added devopsrelease [DEPRECATED] label and removed devopsverify label
mentioned in issue gitlab-development-kit#403
@tuomoa merge cutoff is the 7th, which falls on a sunday this month. So reasonably speaking, today is the last day.
Some documentation for this would be great. Pages administration documentation should live in ce
I've got the start of some docs in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22146 - it details what is now, rather than what should be, though!
Edited by Nick Thomasadded 1 commit
- 4f051c4a - Fix the default test to not expect auth and fix custom config to set access control to true
added 1 commit
- 64912855 - Fix the default test to not expect auth and fix custom config to set access control to true
added 1 commit
- c004f95d - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 77f29e7a - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 84aa56ed - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- 2668a690 - Only authorize pages if access control is enabled, add missing access control…
added 1 commit
- b0a9d707 - Only authorize pages if access control is enabled, add missing access control…