Skip to content
Snippets Groups Projects

Guarantee functions are not lost after Knative update

What does this MR do?

This MR not only renames Cluster::Concerns::ApplicationStatus.installed to Cluster::Concerns::ApplicationStatus.available but also include applications with the state udpated to it, so that functions are properly found for Knative installed and updated apps. Explanation below:

After updating Knative, the app state will be updated instead of installed.

In our FunctionsController we find our functions with the Projects::Serverless::FunctionsFinder:

def finder
  Projects::Serverless::FunctionsFinder.new(project.clusters)
end

This finder will try to find the functions for clusters that are included in the Clusters::Cluster.with_knative_installed:

def execute
  knative_services.flatten.compact
end

def knative_service(environment_scope, name)
  clusters_with_knative_installed.preload_knative.map do |cluster|
  ...
end

def clusters_with_knative_installed
  @clusters.with_knative_installed
end

ultimately, with_knative_installed clusters scope will query clusters that have knative applications that are included in the Clusters::Applications::Knative.installedscope.

scope :with_knative_installed, -> { joins(:application_knative).merge(Clusters::Applications::Knative.installed) }

This change could also fix our Gitlab::UsageData.system_usage_data which also makes use of the above scope to count the number of installed Knative apps.

What are the relevant issue numbers?

#56937 (closed)

Does this MR meet the acceptance criteria?

Edited by Kamil Trzciński

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • @Alexand Thanks for this great MR - I really like the detailed MR description which was a great help in working out what has changed. :100:

    I have a few minor comments above, please take a look ?

    we can then remove this temporary fix and also bring back the ApplicationStatus.installed scope.

    Regarding this, I'm not sure if we even need .installed anymore ? Though I might be missing something here :)

  • assigned to @Alexand

  • added 1 commit

    • b96d28bf - Refactor .available spec into a shared_example

    Compare with previous version

  • added 1 commit

    • 4b9eeeec - Refactor .available spec into a shared_example

    Compare with previous version

  • added 1 commit

    • 26c7d04c - Refactor .available spec into a shared_example

    Compare with previous version

  • João Alexandre Cunha changed title from {-Change Cluster::Concerns::ApplicationStatus.installed so-} functions are not lost after Knative update to Guarantee functions are not lost after Knative update

    changed title from {-Change Cluster::Concerns::ApplicationStatus.installed so-} functions are not lost after Knative update to Guarantee functions are not lost after Knative update

  • @tkuah

    Regarding this, I'm not sure if we even need .installed anymore ? Though I might be missing something here :)

    What I meant is that, according to discussions on issue https://gitlab.com/gitlab-org/gitlab-ce/issues/56524, we were leaning towards removing the updated status in favor or bringing back the installed status. If this becomes a reality in the future, we can also rename the .available scope back to .installed. Actually we can basically revert this whole MR. =]

    Furthermore, thank you very much for the review! I abided by all the suggestions. So, I'm assigning the MR back to you for one more check.

  • Thong Kuah resolved all discussions

    resolved all discussions

  • Thanks @Alexand - LGTM. Please pass on to a maintainer (https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce).

    I see what you mean now regarding renaming back to .installed if we merged #updated into #installed. As this MR also contains valuable improvements, I would suggest creating a new MR to simply rename. But that's for the future!

    Edited by Thong Kuah
  • assigned to @Alexand

  • Thank you for taking the time to do this the review @tkuah. :bow_tone1:

    @ayufan , since you're aware of recent discussions about this subject, could you kindly review this MR? :pray_tone2:

  • Thong Kuah marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • Kamil Trzciński marked the checklist item Tests added for this feature/bug as completed

    marked the checklist item Tests added for this feature/bug as completed

  • Kamil Trzciński marked the checklist item Conforms to the code review guidelines as completed

    marked the checklist item Conforms to the code review guidelines as completed

  • Kamil Trzciński marked the checklist item Conforms to the merge request performance guidelines as completed

    marked the checklist item Conforms to the merge request performance guidelines as completed

  • Kamil Trzciński marked the checklist item Conforms to the style guides as completed

    marked the checklist item Conforms to the style guides as completed

  • Kamil Trzciński marked the checklist item Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. as completed

    marked the checklist item Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. as completed

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading