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.installed
scope.
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?
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Merge request reports
Activity
changed milestone to %11.9
added Category:Kubernetes Management Deliverable Persona: DevOps Engineer Persona: Software developer UX backend devopsconfigure [DEPRECATED] frontend ~2975006 + 1 deleted label
assigned to @Alexand
added 4 commits
Toggle commit list3 Warnings The title of this merge request is longer than 72 characters and would violate our commit message rules when using the Squash on Merge feature. Please consider adjusting the title, or rebase the commits manually and don’t use Squash on Merge. 21ba7313: This commit’s subject line is acceptable, but please try to reduce it to 50 characters. 086ff564: This commit’s subject line is acceptable, but please try to reduce it to 50 characters. Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 1 commit
- 086ff564 - Applications that are installed could also be updated
added 1 commit
- 3fb82525 - Rename scope to imply :installed and :updated result
mentioned in issue #56524 (moved)
added 1 commit
- 21ba7313 - Rename scope to imply :installed and :updated result
mentioned in issue #56937 (closed)
@tkuah , I've discussed internally with @DylanGriffith and we agreed that, since we'll need UX and FE workforce to get rid of the
updated
status and we're short on resources, we could merge this as a temporary fix already. Then once we start working on bringing back theinstalled
final status, we can then remove this temporary fix and also bring back theApplicationStatus.installed
scope.Do you agree? If yes, could you kindly review this MR?
Edited by João Alexandre Cunhaassigned to @tkuah
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by Thong Kuah
- Resolved by 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.
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
added 1 commit
- 4b9eeeec - Refactor .available spec into a shared_example
added 1 commit
- 26c7d04c - Refactor .available spec into a shared_example
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 theinstalled
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.
assigned to @tkuah
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 Kuahassigned to @Alexand
marked the checklist item Changelog entry added, if necessary as completed
marked the checklist item Tests added for this feature/bug as completed
marked the checklist item Conforms to the code review guidelines as completed
marked the checklist item Conforms to the merge request performance guidelines as completed
marked the checklist item Conforms to the style guides 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