-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BC-6420 - unsynced users deletion #4868
Changes from 74 commits
42cf9e1
bd982fb
a7f6cfc
438a0a2
a243c10
b289240
f5efadd
bdffe26
27f053e
16f8958
b0c9472
f278c46
5eaeaa2
f9ecc80
c0e4302
f34eafb
161dc2a
bc23aa8
d3e9050
667c40d
ad33efb
d8ed8fb
a198901
99dd7d0
98cd070
9d49585
32c9652
54a5f27
05942b2
dc7ac4b
9c5d26a
fe5aabc
219ac19
63ec575
e6a8cf1
b20766e
e484758
804ab65
2f03180
f4c62d5
84d0dd9
99ee885
e8bab90
87e5c90
f8c50ab
740c772
4399232
981f177
b5a82a7
4f90380
7359efd
257d7e7
3dc6a80
9aa696f
046a930
b3df782
24bb4c5
205412d
a25a0db
58704c6
dcbba58
6de977e
b473b77
c806112
49d3ede
585230e
4a6fb24
61f7746
efa9ca1
1b2dcc7
7331efb
7b67223
2c7a8ac
3b7e2a0
5cb2e3a
074568b
3bf0bc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
galaxy_info: | ||
role_name: moin-schule-sync | ||
author: Schul-Cloud Verbund | ||
description: moin-schule-sync role for the moin.schule synchronization purposes | ||
company: Schul-Cloud Verbund | ||
license: license (AGPLv3) | ||
min_ansible_version: 2.8 | ||
galaxy_tags: [] | ||
dependencies: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
- name: moin-schule-sync secret provisioned by 1Password | ||
when: WITH_MOIN_SCHULE is defined and WITH_MOIN_SCHULE|bool == true | ||
kubernetes.core.k8s: | ||
kubeconfig: ~/.kube/config | ||
namespace: "{{ NAMESPACE }}" | ||
template: moin-schule-sync-onepassword.yml.j2 | ||
|
||
- name: unsynced moin.schule users deletion queueing CronJob | ||
when: WITH_MOIN_SCHULE is defined and WITH_MOIN_SCHULE|bool == true and WITH_UNSYNCED_ENTITIES_DELETION is defined and WITH_UNSYNCED_ENTITIES_DELETION|bool == true | ||
kubernetes.core.k8s: | ||
kubeconfig: ~/.kube/config | ||
namespace: "{{ NAMESPACE }}" | ||
template: moin-schule-users-deletion-queueing-cronjob.yml.j2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
apiVersion: onepassword.com/v1 | ||
kind: OnePasswordItem | ||
metadata: | ||
name: moin-schule-sync-secret | ||
namespace: {{ NAMESPACE }} | ||
labels: | ||
app: moin-schule-sync | ||
spec: | ||
itemPath: "vaults/{{ ONEPASSWORD_OPERATOR_VAULT }}/items/moin-schule-sync" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
apiVersion: batch/v1 | ||
kind: CronJob | ||
metadata: | ||
namespace: {{ NAMESPACE }} | ||
labels: | ||
app: moin-schule-users-deletion-queueing-cronjob | ||
app.kubernetes.io/part-of: schulcloud-verbund | ||
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }} | ||
app.kubernetes.io/name: moin-schule-users-deletion-queueing-cronjob | ||
app.kubernetes.io/component: sync | ||
app.kubernetes.io/managed-by: ansible | ||
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }} | ||
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }} | ||
name: moin-schule-users-deletion-queueing-cronjob | ||
spec: | ||
schedule: "{{ MOIN_SCHULE_USERS_DELETION_QUEUEING_CRONJOB_SCHEDULE|default("@hourly", true) }}" | ||
jobTemplate: | ||
spec: | ||
template: | ||
spec: | ||
containers: | ||
- name: moin-schule-users-deletion-queueing-cronjob | ||
image: {{ SCHULCLOUD_SERVER_IMAGE }}:{{ SCHULCLOUD_SERVER_IMAGE_TAG }} | ||
envFrom: | ||
- secretRef: | ||
name: moin-schule-sync-secret | ||
command: ['/bin/sh','-c'] | ||
args: ['npm run nest:start:deletion-console -- queue unsynced --systemId $SYSTEM_ID'] | ||
resources: | ||
limits: | ||
cpu: {{ API_CPU_LIMITS|default("2000m", true) }} | ||
memory: {{ API_MEMORY_LIMITS|default("2Gi", true) }} | ||
requests: | ||
cpu: {{ API_CPU_REQUESTS|default("100m", true) }} | ||
memory: {{ API_MEMORY_REQUESTS|default("150Mi", true) }} | ||
restartPolicy: OnFailure | ||
{% if AFFINITY_ENABLE is defined and AFFINITY_ENABLE|bool %} | ||
affinity: | ||
podAffinity: | ||
preferredDuringSchedulingIgnoredDuringExecution: | ||
- weight: 100 | ||
podAffinityTerm: | ||
labelSelector: | ||
matchExpressions: | ||
- key: app.kubernetes.io/part-of | ||
operator: In | ||
values: | ||
- schulcloud-verbund | ||
topologyKey: "kubernetes.io/hostname" | ||
namespaceSelector: {} | ||
{% endif %} | ||
metadata: | ||
labels: | ||
app: moin-schule-users-deletion-queueing-cronjob | ||
app.kubernetes.io/part-of: schulcloud-verbund | ||
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }} | ||
app.kubernetes.io/name: moin-schule-users-deletion-queueing-cronjob | ||
app.kubernetes.io/component: sync | ||
app.kubernetes.io/managed-by: ansible | ||
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }} | ||
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import { ObjectId } from 'bson'; | ||
import { UnsyncedEntitiesOptions } from '../interface'; | ||
import { UnsyncedEntitiesOptionsBuilder } from './unsynced-entities-options.builder'; | ||
|
||
describe(UnsyncedEntitiesOptionsBuilder.name, () => { | ||
describe(UnsyncedEntitiesOptionsBuilder.build.name, () => { | ||
describe('when called with valid arguments', () => { | ||
const setup = () => { | ||
const systemId = new ObjectId().toHexString(); | ||
const unsyncedForMinutes = 3600; | ||
const targetRefDomain = 'school'; | ||
const deleteInMinutes = 43200; | ||
const callsDelayMs = 100; | ||
|
||
const expectedOutput: UnsyncedEntitiesOptions = { | ||
systemId, | ||
unsyncedForMinutes, | ||
targetRefDomain, | ||
deleteInMinutes, | ||
callsDelayMs, | ||
}; | ||
|
||
return { | ||
systemId, | ||
unsyncedForMinutes, | ||
targetRefDomain, | ||
deleteInMinutes, | ||
callsDelayMs, | ||
expectedOutput, | ||
}; | ||
}; | ||
|
||
it('should return valid options object with expected values', () => { | ||
const { systemId, unsyncedForMinutes, targetRefDomain, deleteInMinutes, callsDelayMs, expectedOutput } = | ||
setup(); | ||
|
||
const output = UnsyncedEntitiesOptionsBuilder.build( | ||
systemId, | ||
unsyncedForMinutes, | ||
targetRefDomain, | ||
deleteInMinutes, | ||
callsDelayMs | ||
); | ||
|
||
expect(output).toStrictEqual(expectedOutput); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { EntityId } from '@shared/domain/types'; | ||
import { UnsyncedEntitiesOptions } from '../interface'; | ||
|
||
export class UnsyncedEntitiesOptionsBuilder { | ||
static build( | ||
systemId: EntityId, | ||
unsyncedForMinutes: number, | ||
targetRefDomain?: string, | ||
deleteInMinutes?: number, | ||
callsDelayMs?: number | ||
): UnsyncedEntitiesOptions { | ||
return { | ||
systemId, | ||
unsyncedForMinutes, | ||
targetRefDomain, | ||
deleteInMinutes, | ||
callsDelayMs, | ||
}; | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done but lot of circular dependecies were present so added some services, repos to providers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding Services as Providers directly is NOT the solution to circular dependencies. It does not solve them, it only makes them more narrow and harder to detect. They seem to be solved, but they are not, they will come back to bite us later. Why do these circular dependencies appear? there should be no other module with any dependency on deletion console right? Possibly they are caused by other circular dependencies that are already in the system, but hidden because other people tried to trick them by hiding them in explicit imports. Thats exactly the reason why you cant provide the UserService in your module, it will cause an dependency cycle in some harmless change later down the line in a different PR that has nothing to do with this one. We need to figure out where the circular dependencies come from, what modules actually form the circle (modules, not files), and then either untangling or avoid using them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Metauriel I think you referred to the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
export * from './push-delete-requests-options.interface'; | ||
export * from './trigger-deletion-execution-options.interface'; | ||
export * from './unsynced-entities-options.interface'; | ||
export * from './deletion-execution-trigger-status.enum'; | ||
export * from './deletion-execution-trigger-result'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { EntityId } from '@shared/domain/types'; | ||
|
||
export interface UnsyncedEntitiesOptions { | ||
systemId: EntityId; | ||
unsyncedForMinutes: number; | ||
targetRefDomain?: string; | ||
deleteInMinutes?: number; | ||
callsDelayMs?: number; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these services need to go, if you need them import the modules instead.
Some of the Repos are an exception since they are not part of a module, but you should avoid using the repos at all directly, and instead introduce service methods for things you need to do from the outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 074568b - all the non-local dependencies were moved to
imports
, inproviders
there are just the local dependencies now.