CI status request when project isnt setup for CI

Resources

FE @brycepj

When you are on a merge request requests are being made to the CI status endpoint even if the project doesn't have CI enabled.

We should probably stop any requests to ci_status & environment endpoints for projects/merge requests that do not have CI enabled.

/cc @filipa

You can see this in action on the following merge request:

• https://gitlab.com/issue-reproduce/ci-status-no-ci/merge_requests/1

added ~19173 ~13921 labels

@jschatz1 can we put this in 9.0?

changed milestone to %9.0

We should!

added ~18308 label

assigned to @filipa

Need help to fix this one @ayufan

How do I check if CI is enabled? We have this:

def has_ci?
    source_project.try(:ci_service) && commits.any?
  end

is this enough?

@filipa Not sure if I understand problem correctly, but when you disable pipelines in the project settings, you can read this information through project.builds_enabled?.

@grzesiek I need to know if a MR has CI enabled. I need your help :)

Would project.builds_enabled? be enough?

Here is Slack discussion :

filipa
is this enough to verify if a MR has ci enabled?
`@merge_request.has_ci?`

kamil 
@filipa not sure, I would chekc that, but I assume that not

filipa 

def has_ci?
    source_project.try(:ci_service) && commits.any?
  end


@filipa you could maybe improve that check :slightly_smiling_face: Check if the `.gitlab-ci.yml` is in the project

[6:58]  
That is a cached property, so should be fast

filipa
:grin: I don’t know how to do that

zj 
Ill check
The last check, `commits.any?` is because we can have a MR without changes. Just so you know

This actually is the best we can do :stuck_out_tongue:
I wanted to say, the project should check this! But you can have an MR that introduces it. Futhermore, checking if the current MR includes a CI file isn't cached, so slow. So although this is not perfect, it will have to do

@filipa ^
 
Only improvement would be to write
`source_project&.ci_service && commits.any?`

grzegorz 
@filipa @zj I think that it depends in which context we need this information. CI can be enabled for merge requests via external CI service too. But we also have internal global configuration option for project (`project.builds_enabled?`).

filipa 
basically we request ci_status endpoint in the MR view even when we don’t have CI enabled in the project
what would be the best way in this case?

zj
`target_project.builds_enabled? || target_project&.ci_service` might be good enough? (edited)

grzegorz 
The question is - should you see the external pipeline status even when "Pipelines" feature is disabled for the project? This is probably question to product people /cc @joshua

I mean this is confusing that you can have external CI service +  pipelines disabled, and I'm not completely sure that we are consistent about the expected behavior throughout the codebase.

@filipa I think that implementing has_ci? method as below (suggested by @zj) would make sense.

def has_ci?
  source_project.builds_enabled? || source_project&.ci_service
end

What do you think @zi?

Sorry for the late response, but given the limitations it seems to make sense. Maybe it needs a comment to explain why it was done the way its done and to stress its just a heuristic.

Also, nitpick: In the first statement you expect the source_project not to be nil, the second expression in the || clause you say it might be.

def has_ci?
  source_project&.builds_enabled? || source_project&.ci_service
end

Thank you @zj and @grzesiek 💪

Will add this verification before making request to the ci_status endpoint!

I don't think this is enough, in a repo with no .yml file and without CI this method returns true. I am going to hand over this one to a backend person first 😓

We need to have a method that verifies if the project has CI enabled

added ~901060 label

assigned to @ayufan

@filipa MR.has_ci? was recently updated to take into account GitLab CI in addtion to an external CI service/integration, See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9245/diffs#ef51b30fcc35462117070f3fe498d3949c279cb9_686_686 and is probably what you want to use over source_project.builds_enabled?

By default GitLab CI is enabled. For reference, here is how to disable, http://docs.gitlab.com/ee/ci/enable_or_disable_ci.html#per-project-user-setting

@markpundsack I recall that you commented about this issue in another issue and you felt having pipelines enabled should not be a requirement for using external build status?

Is that really what you said, and if so, can we cross-link these issues, please?

@dblessing I said it "shouldn't" be a requirement, but I can't confirm whether it is currently a requirement. We may have inadvertently changed something when we dropped the Builds tab, or maybe it always required CI/CD to be enabled.

@ayufan This is in 9.0 milestone. Was it shipped but forgot to close the issue? Or has it been forgotten about completely?

@markpundsack Forgotten, we did not yet revisit support for external CI services.

changed milestone to %9.2

removed assignee

added Deliverable label

If I'm not mistaken, this is fixed as part of the MR widget redesign/refactor:

handleMounted() {
  this.checkStatus();
  this.fetchCIStatus();
  this.initDeploymentsPolling();
  if (this.mr.hasCI) {
    this.initCIPolling();
  }
},

source

assigned to @brycepj

@brycepj I don't think it is since there was still no way to fully do this on the backend, I believe.

@filipa hmm.. this isn't sufficient?

  def has_ci?
    has_ci_integration = source_project.try(:ci_service)
    uses_gitlab_ci = all_pipelines.any?
    (has_ci_integration || uses_gitlab_ci) && commits.any?
  end

source

That's what we're exposing to the frontend. It looks like ☝️ is the updated has_ci @MadLittleMods was talking about (https://gitlab.com/gitlab-org/gitlab-ce/commit/605fff91236bb225bb1bddad116ccae3502930e5#ef51b30fcc35462117070f3fe498d3949c279cb9_686_686)

mentioned in merge request !10319 (merged)

@ayufan this is only on the backend ins't it?

assigned to @ayufan

removed frontend label

closed via merge request !10319 (merged)

mentioned in commit 8db76243

mentioned in commit 1ba06c8c