diff options
36 files changed, 506 insertions, 209 deletions
diff --git a/app/assets/javascripts/ide/components/repo_commit_section.vue b/app/assets/javascripts/ide/components/repo_commit_section.vue index b8dca2709c8..2e7e55a61c5 100644 --- a/app/assets/javascripts/ide/components/repo_commit_section.vue +++ b/app/assets/javascripts/ide/components/repo_commit_section.vue @@ -45,7 +45,7 @@ export default { if (this.lastOpenedFile && this.lastOpenedFile.type !== 'tree') { this.openPendingTab({ file: this.lastOpenedFile, - keyPrefix: this.lastOpenedFile.changed ? stageKeys.unstaged : stageKeys.staged, + keyPrefix: this.lastOpenedFile.staged ? stageKeys.staged : stageKeys.unstaged, }) .then(changeViewer => { if (changeViewer) { diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 6257ee3ae8e..ecf2097dc87 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -120,7 +120,7 @@ } @mixin btn-white { - @include btn-color($white, $border-color, $white-normal, $border-white-normal, $white-dark, $border-gray-dark, $gl-text-color); + @include btn-color($white, $border-color, $white-normal, $border-white-normal, $white-dark, $border-white-normal, $gl-text-color); } @mixin btn-with-margin { @@ -365,7 +365,7 @@ .active { box-shadow: $gl-btn-active-background; - border: 1px solid $border-gray-dark !important; + border: 1px solid $border-white-normal !important; background-color: $btn-active-gray-light !important; } } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index fed2b5c905d..c23623005b0 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -329,7 +329,6 @@ $border-white-normal: darken($white-normal, $darken-border-factor); $border-gray-light: darken($gray-light, $darken-border-factor); $border-gray-normal: darken($gray-normal, $darken-border-factor); $border-gray-normal-dashed: darken($gray-normal, $darken-border-dashed-factor); -$border-gray-dark: darken($white-normal, $darken-border-factor); /* * UI elements @@ -350,13 +349,13 @@ $gl-font-size-small: 12px; $gl-font-size-large: 16px; $gl-font-weight-normal: 400; $gl-font-weight-bold: 600; -$gl-text-color: #2e2e2e; -$gl-text-color-secondary: #707070; -$gl-text-color-tertiary: #919191; +$gl-text-color: $gray-900; +$gl-text-color-secondary: $gray-700; +$gl-text-color-tertiary: $gray-600; $gl-text-color-quaternary: #d6d6d6; -$gl-text-color-inverted: rgba(255, 255, 255, 1); +$gl-text-color-inverted: $white; $gl-text-color-secondary-inverted: rgba(255, 255, 255, 0.85); -$gl-text-color-disabled: #919191; +$gl-text-color-disabled: $gray-600; $gl-grayish-blue: #7f8fa4; $gl-gray-dark: #313236; $gl-gray-light: #5c5c5c; diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 78d4383ce28..11291dad28b 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -475,7 +475,7 @@ } .board-card { - border: 1px solid $border-gray-dark; + border: 1px solid $border-white-normal; box-shadow: 0 1px 2px rgba($issue-boards-card-shadow, 0.3); cursor: pointer; } diff --git a/app/assets/stylesheets/pages/commits.scss b/app/assets/stylesheets/pages/commits.scss index 1a07600769c..230f390aa90 100644 --- a/app/assets/stylesheets/pages/commits.scss +++ b/app/assets/stylesheets/pages/commits.scss @@ -131,7 +131,7 @@ color: $gl-text-color-secondary; padding: 1px $gl-padding-4; cursor: pointer; - border: 1px solid $border-gray-dark; + border: 1px solid $border-white-normal; border-radius: $border-radius-default; margin-left: 5px; font-size: 12px; diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index c48f4b0622e..85fdcb753b4 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -426,7 +426,7 @@ height: $sidebar-toggle-height; margin-left: 0; padding-left: 0; - border-bottom: 1px solid $border-gray-dark; + border-bottom: 1px solid $border-white-normal; } a.gutter-toggle { diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index 142078588df..22b5859e297 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -130,8 +130,8 @@ &.selected { td { background: $white-normal; - border-top: 1px solid $border-gray-dark; - border-bottom: 1px solid $border-gray-dark; + border-top: 1px solid $border-white-normal; + border-bottom: 1px solid $border-white-normal; } } } diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb index 18a171700e9..2a8bc823931 100644 --- a/app/controllers/projects/pages_controller.rb +++ b/app/controllers/projects/pages_controller.rb @@ -10,7 +10,7 @@ class Projects::PagesController < Projects::ApplicationController # rubocop: disable CodeReuse/ActiveRecord def show - @domains = @project.pages_domains.order(:domain) + @domains = @project.pages_domains.order(:domain).present(current_user: current_user) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/controllers/projects/pages_domains_controller.rb b/app/controllers/projects/pages_domains_controller.rb index 5a81a064048..cdf6f5ce828 100644 --- a/app/controllers/projects/pages_domains_controller.rb +++ b/app/controllers/projects/pages_domains_controller.rb @@ -26,6 +26,12 @@ class Projects::PagesDomainsController < Projects::ApplicationController redirect_to project_pages_domain_path(@project, @domain) end + def retry_auto_ssl + PagesDomains::RetryAcmeOrderService.new(@domain.pages_domain).execute + + redirect_to project_pages_domain_path(@project, @domain) + end + def edit redirect_to project_pages_domain_path(@project, @domain) end @@ -82,6 +88,6 @@ class Projects::PagesDomainsController < Projects::ApplicationController end def domain - @domain ||= @project.pages_domains.find_by_domain!(params[:id].to_s) + @domain ||= @project.pages_domains.find_by_domain!(params[:id].to_s).present(current_user: current_user) end end diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index 37d45c5934d..486da2c6b45 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class PagesDomain < ApplicationRecord + include Presentable + VERIFICATION_KEY = 'gitlab-pages-verification-code' VERIFICATION_THRESHOLD = 3.days.freeze SSL_RENEWAL_THRESHOLD = 30.days.freeze @@ -13,6 +15,8 @@ class PagesDomain < ApplicationRecord has_many :acme_orders, class_name: "PagesDomainAcmeOrder" has_many :serverless_domain_clusters, class_name: 'Serverless::DomainCluster', inverse_of: :pages_domain + before_validation :clear_auto_ssl_failure, unless: :auto_ssl_enabled + validates :domain, hostname: { allow_numeric_hostname: true } validates :domain, uniqueness: { case_sensitive: false } validates :certificate, :key, presence: true, if: :usage_serverless? @@ -208,6 +212,10 @@ class PagesDomain < ApplicationRecord Pages::VirtualDomain.new([project], domain: self) end + def clear_auto_ssl_failure + self.auto_ssl_failed = false + end + private def pages_deployed? diff --git a/app/services/pages_domains/retry_acme_order_service.rb b/app/services/pages_domains/retry_acme_order_service.rb new file mode 100644 index 00000000000..ef3d8ce0b67 --- /dev/null +++ b/app/services/pages_domains/retry_acme_order_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module PagesDomains + class RetryAcmeOrderService + attr_reader :pages_domain + + def initialize(pages_domain) + @pages_domain = pages_domain + end + + def execute + updated = pages_domain.with_lock do + next unless pages_domain.auto_ssl_enabled && pages_domain.auto_ssl_failed + + pages_domain.update!(auto_ssl_failed: false) + end + + PagesDomainSslRenewalWorker.perform_async(pages_domain.id) if updated + end + end +end diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 6fc06030d7a..0b23a06f5a9 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -26,7 +26,7 @@ %ul - if dashboard_nav_link?(:groups) %li.d-md-none - = link_to dashboard_groups_path do + = link_to dashboard_groups_path, class: 'dashboard-shortcuts-groups' do = _('Groups') - if dashboard_nav_link?(:activity) = nav_link(path: 'dashboard#activity') do diff --git a/app/views/projects/pages/_list.html.haml b/app/views/projects/pages/_list.html.haml index a9e2cbac890..c116efe521a 100644 --- a/app/views/projects/pages/_list.html.haml +++ b/app/views/projects/pages/_list.html.haml @@ -1,12 +1,11 @@ - verification_enabled = Gitlab::CurrentSettings.pages_domain_verification_enabled? -- if can?(current_user, :update_pages, @project) && @domains.load.any? +- if can?(current_user, :update_pages, @project) && @domains.any? .card .card-header Domains (#{@domains.size}) %ul.list-group.list-group-flush.pages-domain-list{ class: ("has-verification-status" if verification_enabled) } - @domains.each do |domain| - - domain = Gitlab::View::Presenter::Factory.new(domain, current_user: current_user).fabricate! %li.pages-domain-list-item.list-group-item.d-flex.justify-content-between - if verification_enabled - tooltip, status = domain.unverified? ? [s_('GitLabPages|Unverified'), 'failed'] : [s_('GitLabPages|Verified'), 'success'] @@ -35,6 +34,6 @@ %li.list-group-item.bs-callout-warning - details_link_start = "<a href='#{project_pages_domain_path(@project, domain)}'>".html_safe - details_link_end = '</a>'.html_safe - = s_("GitLabPages|Something went wrong while obtaining Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}.").html_safe % { domain: domain.domain, + = s_("GitLabPages|Something went wrong while obtaining the Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}.").html_safe % { domain: domain.domain, link_start: details_link_start, link_end: details_link_end } diff --git a/app/views/projects/pages_domains/_certificate.html.haml b/app/views/projects/pages_domains/_certificate.html.haml index 92d30e0b056..e95841f2867 100644 --- a/app/views/projects/pages_domains/_certificate.html.haml +++ b/app/views/projects/pages_domains/_certificate.html.haml @@ -36,7 +36,7 @@ = _('Certificate') .d-flex.justify-content-between.align-items-center.p-3 %span - = @domain.subject || _('missing') + = @domain.pages_domain.subject || _('missing') = link_to _('Remove'), clean_certificate_project_pages_domain_path(@project, @domain), data: { confirm: _('Are you sure?') }, diff --git a/app/views/projects/pages_domains/_lets_encrypt_callout.html.haml b/app/views/projects/pages_domains/_lets_encrypt_callout.html.haml index d6406a78fca..f2de42b218c 100644 --- a/app/views/projects/pages_domains/_lets_encrypt_callout.html.haml +++ b/app/views/projects/pages_domains/_lets_encrypt_callout.html.haml @@ -1,10 +1,21 @@ - if @domain.enabled? - - if @domain.auto_ssl_enabled && !@domain.certificate - .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) } - .row - .col-sm-10.offset-sm-2 - .bs-callout.bs-callout-info.mt-0 - = _("GitLab is obtaining a Let's Encrypt SSL certificate for this domain. This process can take some time. Please try again later.") + - if @domain.auto_ssl_enabled + - if @domain.show_auto_ssl_failed_warning? + .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) } + .row + .col-sm-10.offset-sm-2 + .bs-callout.bs-callout-warning.mt-0 + .row.align-items-center.mx-2 + = icon('warning', class: 'mr-2') + = _("Something went wrong while obtaining the Let's Encrypt certificate.") + .row.mx-0.mt-3 + = link_to s_('GitLabPagesDomains|Retry'), retry_auto_ssl_project_pages_domain_path(@project, @domain), class: "btn btn-sm btn-grouped btn-warning", method: :post + - elsif !@domain.certificate_gitlab_provided? + .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) } + .row + .col-sm-10.offset-sm-2 + .bs-callout.bs-callout-info.mt-0 + = _("GitLab is obtaining a Let's Encrypt SSL certificate for this domain. This process can take some time. Please try again later.") - else .form-group.border-section.js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) } .row diff --git a/changelogs/unreleased/199195-ide-fix-diff-highlighting.yml b/changelogs/unreleased/199195-ide-fix-diff-highlighting.yml new file mode 100644 index 00000000000..439b4348241 --- /dev/null +++ b/changelogs/unreleased/199195-ide-fix-diff-highlighting.yml @@ -0,0 +1,5 @@ +--- +title: Fix Web IDE not showing diff when opening commit tab +merge_request: 29439 +author: +type: fixed diff --git a/changelogs/unreleased/fix-keyboard-shortcut-nav-to-groups.yml b/changelogs/unreleased/fix-keyboard-shortcut-nav-to-groups.yml new file mode 100644 index 00000000000..169d63d941e --- /dev/null +++ b/changelogs/unreleased/fix-keyboard-shortcut-nav-to-groups.yml @@ -0,0 +1,5 @@ +--- +title: Fix keyboard shortcut to navigate to your groups +merge_request: 28873 +author: Victor Wu +type: other diff --git a/changelogs/unreleased/sh-improve-dast-template-error.yml b/changelogs/unreleased/sh-improve-dast-template-error.yml new file mode 100644 index 00000000000..d1943c1e99e --- /dev/null +++ b/changelogs/unreleased/sh-improve-dast-template-error.yml @@ -0,0 +1,5 @@ +--- +title: Improve error message in DAST CI template +merge_request: 29388 +author: +type: other diff --git a/config/routes/project.rb b/config/routes/project.rb index 77183c963d0..ab7c439318f 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -338,6 +338,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do resources :domains, except: :index, controller: 'pages_domains', constraints: { id: %r{[^/]+} } do member do post :verify + post :retry_auto_ssl delete :clean_certificate end end diff --git a/doc/development/code_review.md b/doc/development/code_review.md index c480db54705..52a0672259f 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -13,9 +13,13 @@ You are strongly encouraged to get your code **reviewed** by a [reviewer](https://about.gitlab.com/handbook/engineering/workflow/code-review/#reviewer) as soon as there is any code to review, to get a second opinion on the chosen solution and implementation, and an extra pair of eyes looking for bugs, logic problems, or -uncovered edge cases. The reviewer can be from a different team, but it is -recommended to pick someone who knows the domain well. You can read more about the -importance of involving reviewer(s) in the section on the responsibility of the author below. +uncovered edge cases. + +The default approach is to choose a reviewer from your group or team for the first review. +This is only a recommendation and the reviewer may be from a different team. +However, it is recommended to pick someone who is a [domain expert](#domain-experts). + +You can read more about the importance of involving reviewer(s) in the section on the responsibility of the author below. If you need some guidance (for example, it's your first merge request), feel free to ask one of the [Merge request coaches](https://about.gitlab.com/company/team/). @@ -32,16 +36,32 @@ widget. Reviewers can add their approval by [approving additionally](../user/pro Getting your merge request **merged** also requires a maintainer. If it requires more than one approval, the last maintainer to review and approve it will also merge it. +### Domain experts + +Domain experts are team members who have substantial experience with a specific technology, product feature or area of the codebase. Team members are encouraged to self-identify as domain experts and add it to their [team profile](https://gitlab.com/gitlab-com/www-gitlab-com/-/blob/master/data/team.yml) + +When self-identifying as a domain expert, it is recommended to assign the MR changing the `team.yml` to be merged by an already established Domain Expert or a corresponding Engineering Manager. + +We make the following assumption with regards to automatically being considered a domain expert: + +- Team members working in a specific stage/group (e.g. create: source code) are considered domain experts for that area of the app they work on +- Team members working on a specific feature (e.g. search) are considered domain experts for that feature + +We default to assigning reviews to team members with domain expertise. +When a suitable [domain expert](#domain-experts) isn't available, you can choose any team member to review the MR, or simply follow the [Reviewer roulette](#reviewer-roulette) recommendation. + +Team members' domain expertise can be viewed on the [engineering projects](https://about.gitlab.com/handbook/engineering/projects/) page or on the [GitLab team page](https://about.gitlab.com/company/team/). + ### Reviewer roulette The [Danger bot](dangerbot.md) randomly picks a reviewer and a maintainer for each area of the codebase that your merge request seems to touch. It only makes -recommendations - feel free to override it if you think someone else is a better +**recommendations** and you should override it if you think someone else is a better fit! It picks reviewers and maintainers from the list at the [engineering projects](https://about.gitlab.com/handbook/engineering/projects/) -page, with these behaviours: +page, with these behaviors: 1. It will not pick people whose [GitLab status](../user/profile/index.md#current-status) contains the string 'OOO'. @@ -56,7 +76,7 @@ page, with these behaviours: As described in the section on the responsibility of the maintainer below, you are recommended to get your merge request approved and merged by maintainer(s) -from teams other than your own. + with [domain expertise](#domain-experts). 1. If your merge request includes backend changes [^1], it must be **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_backend)**. @@ -103,13 +123,13 @@ To reach the required level of confidence in their solution, an author is expect to involve other people in the investigation and implementation processes as appropriate. -They are encouraged to reach out to domain experts to discuss different solutions +They are encouraged to reach out to [domain experts](#domain-experts) to discuss different solutions or get an implementation reviewed, to product managers and UX designers to clear up confusion or verify that the end result matches what they had in mind, to database specialists to get input on the data model or specific queries, or to any other developer to get an in-depth review of the solution. -If an author is unsure if a merge request needs a domain expert's opinion, that's +If an author is unsure if a merge request needs a [domain experts's](#domain-experts) opinion, that's usually a pretty good sign that it does, since without it the required level of confidence in their solution will not have been reached. @@ -142,9 +162,8 @@ that it meets all requirements, you should: - Click the Approve button. - Advise the author their merge request has been reviewed and approved. -- Assign the merge request to a maintainer. [Reviewer roulette](#reviewer-roulette) -should have made a suggestion, but feel free to override if someone else is a -better choice. +- Assign the merge request to a maintainer. Default to assigning it to a maintainer with [domain expertise](#domain-experts), +however, if one isn't available or you think the merge request doesn't need a review by a [domain expert](#domain-experts), feel free to follow the [Reviewer roulette](#reviewer-roulette) suggestion. ### The responsibility of the maintainer @@ -159,20 +178,17 @@ Since a maintainer's job only depends on their knowledge of the overall GitLab codebase, and not that of any specific domain, they can review, approve, and merge merge requests from any team and in any product area. -In fact, authors are encouraged to get their merge requests merged by maintainers -from teams other than their own, to ensure that all code across GitLab is consistent -and can be easily understood by all contributors, from both inside and outside the -company, without requiring team-specific expertise. - Maintainers will do their best to also review the specifics of the chosen solution -before merging, but as they are not necessarily domain experts, they may be poorly +before merging, but as they are not necessarily [domain experts](#domain-experts), they may be poorly placed to do so without an unreasonable investment of time. In those cases, they -will defer to the judgment of the author and earlier reviewers and involved domain -experts, in favor of focusing on their primary responsibilities. +will defer to the judgment of the author and earlier reviewers, in favor of focusing on their primary responsibilities. + +If a maintainer feels that an MR is substantial enough that it warrants a review from a [domain expert](#domain-experts), +and it is unclear whether a domain expert have been involved in the reviews to date, +they may request a [domain expert's](#domain-experts) review before merging the MR. If a developer who happens to also be a maintainer was involved in a merge request -as a domain expert and/or reviewer, it is recommended that they are not also picked -as the maintainer to ultimately approve and merge it. +as a reviewer, it is recommended that they are not also picked as the maintainer to ultimately approve and merge it. Maintainers should check before merging if the merge request is approved by the required approvers. @@ -255,11 +271,13 @@ first time. ### Assigning a merge request for a review -If you want to have your merge request reviewed, you can assign it to any reviewer. The list of reviewers can be found on [Engineering projects](https://about.gitlab.com/handbook/engineering/projects/) page. +When you are ready to have your merge request reviewed, +you should default to assigning it to a reviewer from your group or team for the first review, +however, you can also assign it to any reviewer. The list of reviewers can be found on [Engineering projects](https://about.gitlab.com/handbook/engineering/projects/) page. You can also use `workflow::ready for review` label. That means that your merge request is ready to be reviewed and any reviewer can pick it. It is recommended to use that label only if there isn't time pressure and make sure the merge request is assigned to a reviewer. -When your merge request was reviewed and can be passed to a maintainer you can either pick a specific maintainer or use a label `ready for merge`. +When your merge request was reviewed and can be passed to a maintainer, you should default to choosing a maintainer with [domain expertise](#domain-experts), and otherwise follow the Reviewer Roulette recommendation or use the label `ready for merge`. It is responsibility of the author of a merge request that the merge request is reviewed. If it stays in `ready for review` state too long it is recommended to assign it to a specific reviewer. diff --git a/doc/development/testing_guide/end_to_end/best_practices.md b/doc/development/testing_guide/end_to_end/best_practices.md index fbb2a17bef1..57cfcf34726 100644 --- a/doc/development/testing_guide/end_to_end/best_practices.md +++ b/doc/development/testing_guide/end_to_end/best_practices.md @@ -1,43 +1,59 @@ -# Best practices when writing end-to-end tests +# End-to-end testing Best Practices -## Avoid using a GUI when it's not required +NOTE: **Note:** +This is an tailored extension of the Best Practices [found in the testing guide](../best_practices.md). -The majority of the end-to-end tests require some state to be built in the application for the tests to happen. +## Prefer API over UI -A good example is a user being logged in as a pre-condition for testing the feature. +The end-to-end testing framework has the ability to fabricate its resources on a case-by-case basis. +Resources should be fabricated via the API wherever possible. -But if the login feature is already covered with end-to-end tests through the GUI, there is no reason to perform such an expensive task to test the functionality of creating a project, or importing a repo, even if these features depend on a user being logged in. Let's see an example to make things clear. +We can save both time and money by fabricating resources that our test will need via the API. -Let's say that, on average, the process to perform a successful login through the GUI takes 2 seconds. +[Learn more](resources.md) about resources. -Now, realize that almost all tests need the user to be logged in, and that we need every test to run in isolation, meaning that tests cannot interfere with each other. This would mean that for every test the user needs to log in, and "waste 2 seconds". +## Avoid superfluous expectations -Now, multiply the number of tests per 2 seconds, and as your test suite grows, the time to run it grows with it, and this is not sustainable. +To keep tests lean, it is important that we only test what we need to test. -An alternative to perform a login in a cheaper way would be having an endpoint (available only for testing) where we could pass the user's credentials as encrypted values as query strings, and then we would be redirected to the logged in home page if the credentials are valid. Let's say that, on average, this process takes only 200 milliseconds. +Ensure that you do not add any `expect()` statements that are unrelated to what needs to be tested. -You see the point right? +For example: -Performing a login through the GUI for every test would cost a lot in terms of tests' execution. - -And there is another reason. - -Let's say that you don't follow the above suggestion, and depend on the GUI for the creation of every application state in order to test a specific feature. In this case we could be talking about the **Issues** feature, that depends on a project to exist, and the user to be logged in. - -What would happen if there was a bug in the project creation page, where the 'Create' button is disabled, not allowing for the creation of a project through the GUI, but the API logic is still working? - -In this case, instead of having only the project creation test failing, we would have many tests that depend on a project to be failing too. +```ruby +#=> Good +Flow::Login.sign_in +Page::Main::Menu.perform do |menu| + expect(menu).to be_signed_in +end -But, if we were following the best practices, only one test would be failing, and tests for other features that depend on a project to exist would continue to pass, since they could be creating the project behind the scenes interacting directly with the public APIs, ensuring a more reliable metric of test failure rate. +#=> Bad +Flow::Login.sign_in(as: user) +Page::Main::Menu.perform do |menu| + expect(menu).to be_signed_in + expect(page).to have_content(user.name) #=> we already validated being signed in. redundant. + expect(menu).to have_element(:nav_bar) #=> likely unnecessary. already validated in lower-level. test doesn't call for validating this. +end -Finally, interacting with the application only by its GUI generates a higher rate of test flakiness, and we want to avoid that at max. +#=> Good +issue = Resource::Issue.fabricate_via_api! do |issue| + issue.name = 'issue-name' +end -**The takeaways here are:** +Project::Issues::Index.perform do |index| + expect(index).to have_issue(issue) +end -- Building state through the GUI is time consuming and it's not sustainable as the test suite grows. -- When depending only on the GUI to create the application's state and tests fail due to front-end issues, we can't rely on the test failures rate, and we generate a higher rate of test flakiness. +#=> Bad +issue = Resource::Issue.fabricate_via_api! do |issue| + issue.name = 'issue-name' +end -Now that we are aware of all of it, [let's go create some tests](quick_start_guide.md). +Project::Issues::Index.perform do |index| + expect(index).to have_issue(issue) + expect(page).to have_content(issue.name) #=> page content check is redundant as the issue was already validated in the line above. +end +``` ## Prefer to split tests across multiple files @@ -54,17 +70,18 @@ In summary: - **Do**: Split tests across separate files, unless the tests share expensive setup. - **Don't**: Put new tests in an existing file without considering the impact on parallelization. -## Limit the use of `before(:all)` and `after` hooks +## Limit the use of the UI in `before(:context)` and `after` hooks -Limit the use of `before(:all)` hook to perform setup tasks with only API calls, non UI operations -or basic UI operations such as login. +Limit the use of `before(:context)` hooks to perform setup tasks with only API calls, +non-UI operations, or basic UI operations such as login. -We use [`capybara-screenshot`](https://github.com/mattheworiordan/capybara-screenshot) library to automatically save screenshots on failures. -This library [saves the screenshots in the RSpec's `after` hook](https://github.com/mattheworiordan/capybara-screenshot/blob/master/lib/capybara-screenshot/rspec.rb#L97). -[If there is a failure in `before(:all)`, the `after` hook is not called](https://github.com/rspec/rspec-core/pull/2652/files#diff-5e04af96d5156e787f28d519a8c99615R148) and so the screenshots are not saved. +We use [`capybara-screenshot`](https://github.com/mattheworiordan/capybara-screenshot) library to automatically save a screenshot on +failure. -Given this fact, we should limit the use of `before(:all)` to only those operations where a screenshot is not -necessary in case of failure and QA logs would be enough for debugging. +`capybara-screenshot` [saves the screenshot in the RSpec's `after` hook](https://github.com/mattheworiordan/capybara-screenshot/blob/master/lib/capybara-screenshot/rspec.rb#L97). +[If there is a failure in `before(:context)`, the `after` hook is not called](https://github.com/rspec/rspec-core/pull/2652/files#diff-5e04af96d5156e787f28d519a8c99615R148) and so the screenshot is not saved. + +Given this fact, we should limit the use of `before(:context)` to only those operations where a screenshot is not needed. Similarly, the `after` hook should only be used for non-UI operations. Any UI operations in `after` hook in a test file would execute before the `after` hook that takes the screenshot. This would result in moving the UI status away from the @@ -72,16 +89,11 @@ point of failure and so the screenshot would not be captured at the right moment ## Ensure tests do not leave the browser logged in -All QA tests expect to be able to log in at the start of the test. - -That's not possible if a test leaves the browser logged in when it finishes. Normally this isn't a -problem because [Capybara resets the session after each test](https://github.com/teamcapybara/capybara/blob/9ebc5033282d40c73b0286e60217515fd1bb0b5d/lib/capybara/rspec.rb#L18). -But Capybara does that in an `after` block, so when a test logs in within an `after(:context)` block, -the browser returns to a logged in state *after* Capybara had logged it out. And so the next test will fail. +All tests expect to be able to log in at the start of the test. For an example see: <https://gitlab.com/gitlab-org/gitlab/issues/34736> -Ideally, any actions performed in an `after(:context)` (or [`before(:context)`](#limit-the-use-of-beforeall-and-after-hooks)) block would be performed via the API. But if it's necessary to do so via the UI (e.g., if API functionality doesn't exist), make sure to log out at the end of the block. +Ideally, any actions performed in an `after(:context)` (or [`before(:context)`](#limit-the-use-of-the-ui-in-beforecontext-and-after-hooks)) block would be performed via the API. But if it's necessary to do so via the UI (e.g., if API functionality doesn't exist), make sure to log out at the end of the block. ```ruby after(:all) do @@ -100,3 +112,30 @@ We don't run tests that require Administrator access against our Production envi When you add a new test that requires Administrator access, apply the RSpec metadata `:requires_admin` so that the test will not be included in the test suites executed against Production and other environments on which we don't want to run those tests. Note: When running tests locally or configuring a pipeline, the environment variable `QA_CAN_TEST_ADMIN_FEATURES` can be set to `false` to skip tests that have the `:requires_admin` tag. + +## Prefer `Commit` resource over `ProjectPush` + +In line with [using the API](#prefer-api-over-ui), use a `Commit` resource whenever possible. + +`ProjectPush` uses raw shell commands via the Git Command Line Interface (CLI) whereas the `Commit` resource makes an HTTP request. + +```ruby +# Using a commit resource +Resource::Commit.fabricate_via_api! do |commit| + commit.commit_message = 'Initial commit' + commit.add_files([ + {file_path: 'README.md', content: 'Hello, GitLab'} + ]) +end + +# Using a ProjectPush +Resource::Repository::ProjectPush.fabricate! do |push| + push.commit_message = 'Initial commit' + push.file_name = 'README.md' + push.file_content = 'Hello, GitLab' +end +``` + +NOTE: **Note:** +A few exceptions for using a `ProjectPush` would be when your test calls for testing SSH integration or +using the Git CLI. diff --git a/doc/user/clusters/applications.md b/doc/user/clusters/applications.md index f8fd07276c5..73ef9482e71 100644 --- a/doc/user/clusters/applications.md +++ b/doc/user/clusters/applications.md @@ -43,6 +43,7 @@ The following applications can be installed: - [Knative](#knative) - [Crossplane](#crossplane) - [Elastic Stack](#elastic-stack) +- [Fluentd](#fluentd) With the exception of Knative, the applications will be installed in a dedicated namespace called `gitlab-managed-apps`. @@ -523,6 +524,28 @@ kubectl port-forward svc/kibana 5601:443 Then, you can visit Kibana at `http://localhost:5601`. +### Fluentd + +> Introduced in GitLab 12.10 for project- and group-level clusters. + +[Fluentd](https://www.fluentd.org/) is an open source data collector, which enables +you to unify the data collection and consumption to better use and understand +your data. Fluentd sends logs in syslog format. + +To enable Fluentd: + +1. Navigate to **{cloud-gear}** **Operations > Kubernetes** and click + **Applications**. You will be prompted to enter a host, port and protocol + where the WAF logs will be sent to via syslog. +1. Provide the host domain name or URL in **SIEM URL or Host**. +1. Provide the host port number in **SIEM Port**. +1. Select a **SIEM Protocol**. +1. Check **Send ModSecurity Logs**. If you do not select this checkbox, the **Install** + button is disabled. +1. Click **Install**. + + + ### Future apps Interested in contributing a new GitLab managed app? Visit the @@ -552,6 +575,7 @@ Supported applications: - [JupyterHub](#install-jupyterhub-using-gitlab-cicd) - [Elastic Stack](#install-elastic-stack-using-gitlab-cicd) - [Crossplane](#install-crossplane-using-gitlab-cicd) +- [Fluentd](#install-fluentd-using-gitlab-cicd) ### Usage @@ -1036,6 +1060,30 @@ management project. Refer to the [chart](https://github.com/crossplane/crossplane/tree/master/cluster/charts/crossplane#configuration) for the available configuration options. Note that this link points to the docs for the current development release, which may differ from the version you have installed. You can check out a specific version in the branch/tag switcher. +### Install Fluentd using GitLab CI/CD + +> [Introduced](https://gitlab.com/gitlab-org/cluster-integration/cluster-applications/-/merge_requests/76) in GitLab 12.10. + +To install Fluentd into the `gitlab-managed-apps` namespace of your cluster using GitLab CI/CD, define the following configuration in `.gitlab/managed-apps/config.yaml`: + +```yaml +Fluentd: + installed: true +``` + +You can also review the default values set for this chart in the [values.yaml](https://github.com/helm/charts/blob/master/stable/fluentd/values.yaml) file. + +You can customize the installation of Fluentd by defining +`.gitlab/managed-apps/fluentd/values.yaml` file in your cluster management +project. Refer to the +[configuration chart for the current development release of Fluentd](https://github.com/helm/charts/tree/master/stable/fluentd#configuration) +for the available configuration options. + +NOTE: **Note:** +The configuration chart link points to the current development release, which +may differ from the version you have installed. To ensure compatibility, switch +to the specific branch or tag you are using. + ## Upgrading applications > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/24789) in GitLab 11.8. diff --git a/doc/user/clusters/img/fluentd_v12_10.png b/doc/user/clusters/img/fluentd_v12_10.png Binary files differnew file mode 100644 index 00000000000..7593f99ab51 --- /dev/null +++ b/doc/user/clusters/img/fluentd_v12_10.png diff --git a/doc/user/project/pages/custom_domains_ssl_tls_certification/lets_encrypt_integration.md b/doc/user/project/pages/custom_domains_ssl_tls_certification/lets_encrypt_integration.md index 42b1570d213..f80b741fb77 100644 --- a/doc/user/project/pages/custom_domains_ssl_tls_certification/lets_encrypt_integration.md +++ b/doc/user/project/pages/custom_domains_ssl_tls_certification/lets_encrypt_integration.md @@ -60,11 +60,11 @@ associated Pages domain. It also will be renewed automatically by GitLab. ## Troubleshooting -### Error "Something went wrong while obtaining Let's Encrypt certificate" +### Error "Something went wrong while obtaining the Let's Encrypt certificate" > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/30146) in GitLab 13.0. -If you get an error **Something went wrong while obtaining Let's Encrypt certificate**, you can try obtaining the certificate again by following these steps: +If you get an error **Something went wrong while obtaining the Let's Encrypt certificate**, you can try obtaining the certificate again by following these steps: 1. Go to your project's **Settings > Pages**. 1. Click **Edit** on your domain. diff --git a/lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml b/lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml index 10ef33e71d5..0e3d7660bdf 100644 --- a/lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Security/DAST.gitlab-ci.yml @@ -22,6 +22,7 @@ dast: allow_failure: true script: - export DAST_WEBSITE=${DAST_WEBSITE:-$(cat environment_url.txt)} + - if [ -z "$DAST_WEBSITE$DAST_API_SPECIFICATION" ]; then echo "Either DAST_WEBSITE or DAST_API_SPECIFICATION must be set. See https://docs.gitlab.com/ee/user/application_security/dast/#configuration for more details." && exit 1; fi - /analyze artifacts: reports: diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index d95da262eea..706c16f6149 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -57,6 +57,10 @@ module Gitlab v2 ].freeze + # NOTE: Do not add new items to this list unless necessary as this will + # cause conflicts with existing namespaced routes for groups or projects. + # See https://docs.gitlab.com/ee/development/routing.html#project-routes + # # This list should contain all words following `/*namespace_id/:project_id` in # routes that contain a second wildcard. # @@ -103,6 +107,10 @@ module Gitlab wikis ].freeze + # NOTE: Do not add new items to this list unless necessary as this will + # cause conflicts with existing namespaced routes for groups or projects. + # See https://docs.gitlab.com/ee/development/routing.html#group-routes + # # These are all the paths that follow `/groups/*id/ or `/groups/*group_id` # We need to reject these because we have a `/groups/*id` page that is the same # as the `/*id`. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b9863823be0..58d2b4b9586 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9801,6 +9801,9 @@ msgstr "" msgid "GitLab.com import" msgstr "" +msgid "GitLabPagesDomains|Retry" +msgstr "" + msgid "GitLabPages|%{domain} is not verified. To learn how to verify ownership, visit your %{link_start}domain details%{link_end}." msgstr "" @@ -9867,7 +9870,7 @@ msgstr "" msgid "GitLabPages|Save" msgstr "" -msgid "GitLabPages|Something went wrong while obtaining Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}." +msgid "GitLabPages|Something went wrong while obtaining the Let's Encrypt certificate for %{domain}. To retry visit your %{link_start}domain details%{link_end}." msgstr "" msgid "GitLabPages|Support for domains and certificates is disabled. Ask your system's administrator to enable it." @@ -12085,6 +12088,9 @@ msgstr "" msgid "Licenses|Detected in Project" msgstr "" +msgid "Licenses|Detected licenses that are out-of-compliance with the project's assigned policies" +msgstr "" + msgid "Licenses|Displays licenses detected in the project, based on the %{linkStart}latest successful%{linkEnd} scan" msgstr "" @@ -12106,6 +12112,9 @@ msgstr "" msgid "Licenses|Policy" msgstr "" +msgid "Licenses|Policy violation: denied" +msgstr "" + msgid "Licenses|Specified policies in this project" msgstr "" diff --git a/spec/controllers/projects/pages_domains_controller_spec.rb b/spec/controllers/projects/pages_domains_controller_spec.rb index ef5e831d26c..c78c5fe2886 100644 --- a/spec/controllers/projects/pages_domains_controller_spec.rb +++ b/spec/controllers/projects/pages_domains_controller_spec.rb @@ -181,6 +181,24 @@ describe Projects::PagesDomainsController do end end + describe 'POST retry_auto_ssl' do + before do + pages_domain.update!(auto_ssl_enabled: true, auto_ssl_failed: true) + end + + let(:params) { request_params.merge(id: pages_domain.domain) } + + it 'calls retry service and redirects' do + expect_next_instance_of(PagesDomains::RetryAcmeOrderService, pages_domain) do |service| + expect(service).to receive(:execute) + end + + post :retry_auto_ssl, params: params + + expect(response).to redirect_to project_pages_domain_path(project, pages_domain) + end + end + describe 'DELETE destroy' do it "deletes the pages domain" do expect do diff --git a/spec/features/dashboard/shortcuts_spec.rb b/spec/features/dashboard/shortcuts_spec.rb index 2cd9cbc4471..6907c681417 100644 --- a/spec/features/dashboard/shortcuts_spec.rb +++ b/spec/features/dashboard/shortcuts_spec.rb @@ -26,6 +26,10 @@ describe 'Dashboard shortcuts', :js do check_page_title('To-Do List') + find('body').send_keys([:shift, 'G']) + + check_page_title('Groups') + find('body').send_keys([:shift, 'P']) check_page_title('Projects') diff --git a/spec/features/projects/pages_lets_encrypt_spec.rb b/spec/features/projects/pages_lets_encrypt_spec.rb index 4f9c1903344..da9b191271a 100644 --- a/spec/features/projects/pages_lets_encrypt_spec.rb +++ b/spec/features/projects/pages_lets_encrypt_spec.rb @@ -85,6 +85,22 @@ describe "Pages with Let's Encrypt", :https_pages_enabled do end end + context "when we failed to obtain Let's Encrypt certificate", :js do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true, project: project) + end + + it 'user can retry obtaining certificate' do + visit project_pages_domain_path(project, domain) + + expect(page).to have_text("Something went wrong while obtaining the Let's Encrypt certificate.") + + click_on('Retry') + + expect(page).to have_text("GitLab is obtaining a Let's Encrypt SSL certificate for this domain. This process can take some time. Please try again later.") + end + end + shared_examples 'user sees private keys only for user provided certificate' do shared_examples 'user do not see private key' do it 'user do not see private key' do diff --git a/spec/frontend/ide/components/repo_commit_section_spec.js b/spec/frontend/ide/components/repo_commit_section_spec.js new file mode 100644 index 00000000000..5ea03eb1593 --- /dev/null +++ b/spec/frontend/ide/components/repo_commit_section_spec.js @@ -0,0 +1,134 @@ +import { mount } from '@vue/test-utils'; +import { createStore } from '~/ide/stores'; +import router from '~/ide/ide_router'; +import RepoCommitSection from '~/ide/components/repo_commit_section.vue'; +import { stageKeys } from '~/ide/constants'; +import { file } from '../helpers'; + +const TEST_NO_CHANGES_SVG = 'nochangessvg'; + +describe('RepoCommitSection', () => { + let wrapper; + let store; + + function createComponent() { + wrapper = mount(RepoCommitSection, { store }); + } + + function setupDefaultState() { + store.state.noChangesStateSvgPath = 'svg'; + store.state.committedStateSvgPath = 'commitsvg'; + store.state.currentProjectId = 'abcproject'; + store.state.currentBranchId = 'master'; + store.state.projects.abcproject = { + web_url: '', + branches: { + master: { + workingReference: '1', + }, + }, + }; + + const files = [file('file1'), file('file2')].map(f => + Object.assign(f, { + type: 'blob', + content: 'orginal content', + }), + ); + + store.state.rightPanelCollapsed = false; + store.state.currentBranch = 'master'; + store.state.changedFiles = []; + store.state.stagedFiles = [{ ...files[0] }, { ...files[1] }]; + store.state.stagedFiles.forEach(f => + Object.assign(f, { + changed: true, + staged: true, + content: 'testing', + }), + ); + + files.forEach(f => { + store.state.entries[f.path] = f; + }); + } + + beforeEach(() => { + store = createStore(); + + jest.spyOn(store, 'dispatch'); + jest.spyOn(router, 'push').mockImplementation(); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('empty Stage', () => { + beforeEach(() => { + store.state.noChangesStateSvgPath = TEST_NO_CHANGES_SVG; + store.state.committedStateSvgPath = 'svg'; + + createComponent(); + }); + + it('renders no changes text', () => { + expect( + wrapper + .find('.js-empty-state') + .text() + .trim(), + ).toContain('No changes'); + expect(wrapper.find('.js-empty-state img').attributes('src')).toBe(TEST_NO_CHANGES_SVG); + }); + }); + + describe('default', () => { + beforeEach(() => { + setupDefaultState(); + + createComponent(); + }); + + it('opens last opened file', () => { + expect(store.state.openFiles.length).toBe(1); + expect(store.state.openFiles[0].pending).toBe(true); + }); + + it('calls openPendingTab', () => { + expect(store.dispatch).toHaveBeenCalledWith('openPendingTab', { + file: store.getters.lastOpenedFile, + keyPrefix: stageKeys.staged, + }); + }); + + it('renders a commit section', () => { + const allFiles = store.state.changedFiles.concat(store.state.stagedFiles); + const changedFileNames = wrapper + .findAll('.multi-file-commit-list > li') + .wrappers.map(x => x.text().trim()); + + expect(changedFileNames).toEqual(allFiles.map(x => x.path)); + }); + }); + + describe('with unstaged file', () => { + beforeEach(() => { + setupDefaultState(); + + store.state.changedFiles = store.state.stagedFiles.map(x => + Object.assign(x, { staged: false }), + ); + store.state.stagedFiles = []; + + createComponent(); + }); + + it('calls openPendingTab with unstaged prefix', () => { + expect(store.dispatch).toHaveBeenCalledWith('openPendingTab', { + file: store.getters.lastOpenedFile, + keyPrefix: stageKeys.unstaged, + }); + }); + }); +}); diff --git a/spec/javascripts/ide/components/repo_commit_section_spec.js b/spec/javascripts/ide/components/repo_commit_section_spec.js deleted file mode 100644 index 0ba8c86a036..00000000000 --- a/spec/javascripts/ide/components/repo_commit_section_spec.js +++ /dev/null @@ -1,113 +0,0 @@ -import Vue from 'vue'; -import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; -import store from '~/ide/stores'; -import router from '~/ide/ide_router'; -import repoCommitSection from '~/ide/components/repo_commit_section.vue'; -import { file, resetStore } from '../helpers'; - -describe('RepoCommitSection', () => { - let vm; - - function createComponent() { - const Component = Vue.extend(repoCommitSection); - - store.state.noChangesStateSvgPath = 'svg'; - store.state.committedStateSvgPath = 'commitsvg'; - - vm = createComponentWithStore(Component, store); - - vm.$store.state.currentProjectId = 'abcproject'; - vm.$store.state.currentBranchId = 'master'; - vm.$store.state.projects.abcproject = { - web_url: '', - branches: { - master: { - workingReference: '1', - }, - }, - }; - - const files = [file('file1'), file('file2')].map(f => - Object.assign(f, { - type: 'blob', - content: 'orginal content', - }), - ); - - vm.$store.state.rightPanelCollapsed = false; - vm.$store.state.currentBranch = 'master'; - vm.$store.state.changedFiles = []; - vm.$store.state.stagedFiles = [{ ...files[0] }, { ...files[1] }]; - vm.$store.state.stagedFiles.forEach(f => - Object.assign(f, { - changed: true, - content: 'testing', - }), - ); - - files.forEach(f => { - vm.$store.state.entries[f.path] = f; - }); - - return vm; - } - - beforeEach(done => { - spyOn(router, 'push'); - - vm = createComponent(); - - spyOn(vm, 'openPendingTab').and.callThrough(); - - vm.$mount(); - - Vue.nextTick(done); - }); - - afterEach(() => { - vm.$destroy(); - - resetStore(vm.$store); - }); - - describe('empty Stage', () => { - it('renders no changes text', () => { - resetStore(vm.$store); - const Component = Vue.extend(repoCommitSection); - - store.state.noChangesStateSvgPath = 'nochangessvg'; - store.state.committedStateSvgPath = 'svg'; - - vm.$destroy(); - vm = createComponentWithStore(Component, store).$mount(); - - expect(vm.$el.querySelector('.js-empty-state').textContent.trim()).toContain('No changes'); - expect(vm.$el.querySelector('.js-empty-state img').getAttribute('src')).toBe('nochangessvg'); - }); - }); - - it('renders a commit section', () => { - const changedFileElements = [...vm.$el.querySelectorAll('.multi-file-commit-list > li')]; - const allFiles = vm.$store.state.changedFiles.concat(vm.$store.state.stagedFiles); - - expect(changedFileElements).toHaveLength(2); - - changedFileElements.forEach((changedFile, i) => { - expect(changedFile.textContent.trim()).toContain(allFiles[i].path); - }); - }); - - describe('mounted', () => { - it('opens last opened file', () => { - expect(store.state.openFiles.length).toBe(1); - expect(store.state.openFiles[0].pending).toBe(true); - }); - - it('calls openPendingTab', () => { - expect(vm.openPendingTab).toHaveBeenCalledWith({ - file: vm.lastOpenedFile, - keyPrefix: 'unstaged', - }); - }); - }); -}); diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 4bf56e7b28b..fa2648979e9 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -536,6 +536,24 @@ describe PagesDomain do 'user_provided', 'gitlab_provided') end + describe '#save' do + context 'when we failed to obtain ssl certificate' do + let(:domain) { create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true) } + + it 'clears failure if auto ssl is disabled' do + expect do + domain.update!(auto_ssl_enabled: false) + end.to change { domain.auto_ssl_failed }.from(true).to(false) + end + + it 'does not clear failure on unrelated updates' do + expect do + domain.update!(verified_at: Time.now) + end.not_to change { domain.auto_ssl_failed }.from(true) + end + end + end + describe '.for_removal' do subject { described_class.for_removal } diff --git a/spec/services/pages_domains/retry_acme_order_service_spec.rb b/spec/services/pages_domains/retry_acme_order_service_spec.rb new file mode 100644 index 00000000000..0185f10864c --- /dev/null +++ b/spec/services/pages_domains/retry_acme_order_service_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PagesDomains::RetryAcmeOrderService do + let(:domain) { create(:pages_domain, auto_ssl_enabled: true, auto_ssl_failed: true) } + + let(:service) { described_class.new(domain) } + + it 'clears auto_ssl_failed' do + expect do + service.execute + end.to change { domain.auto_ssl_failed }.from(true).to(false) + end + + it 'schedules renewal worker' do + expect(PagesDomainSslRenewalWorker).to receive(:perform_async).with(domain.id).and_return(nil).once + + service.execute + end + + it "doesn't schedule renewal worker if Let's Encrypt integration is not enabled" do + domain.update!(auto_ssl_enabled: false) + + expect(PagesDomainSslRenewalWorker).not_to receive(:new) + + service.execute + end + + it "doesn't schedule renewal worker if auto ssl has not failed yet" do + domain.update!(auto_ssl_failed: false) + + expect(PagesDomainSslRenewalWorker).not_to receive(:new) + + service.execute + end +end diff --git a/spec/views/projects/pages/show.html.haml_spec.rb b/spec/views/projects/pages/show.html.haml_spec.rb index 39384484279..63b66616f31 100644 --- a/spec/views/projects/pages/show.html.haml_spec.rb +++ b/spec/views/projects/pages/show.html.haml_spec.rb @@ -17,7 +17,7 @@ describe 'projects/pages/show' do assign(:project, project) allow(view).to receive(:current_user).and_return(user) - assign(:domains, project.pages_domains) + assign(:domains, [domain.present(current_user: user)]) end describe 'validation warning' do @@ -47,7 +47,7 @@ describe 'projects/pages/show' do describe "warning about failed Let's Encrypt" do let(:error_message) do - "Something went wrong while obtaining Let's Encrypt certificate for #{domain.domain}. "\ + "Something went wrong while obtaining the Let's Encrypt certificate for #{domain.domain}. "\ "To retry visit your domain details." end diff --git a/spec/views/projects/pages_domains/show.html.haml_spec.rb b/spec/views/projects/pages_domains/show.html.haml_spec.rb index 51c7a08fe96..7d502e74d10 100644 --- a/spec/views/projects/pages_domains/show.html.haml_spec.rb +++ b/spec/views/projects/pages_domains/show.html.haml_spec.rb @@ -7,7 +7,7 @@ describe 'projects/pages_domains/show' do before do assign(:project, project) - assign(:domain, domain) + assign(:domain, domain.present) stub_pages_setting(external_https: true) end |
