Skip to content
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

Upgrade runtime #227

Merged
merged 28 commits into from
Aug 15, 2024
Merged

Upgrade runtime #227

merged 28 commits into from
Aug 15, 2024

Conversation

Magnus-Kuhn
Copy link
Contributor

@Magnus-Kuhn Magnus-Kuhn commented Aug 2, 2024

Readiness checklist

  • I added/updated tests.
  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.

@Magnus-Kuhn Magnus-Kuhn added dependencies Pull requests that update dependencies runtime-upgrade Pull requests that update the runtime labels Aug 2, 2024
@Magnus-Kuhn Magnus-Kuhn changed the title Chore/upgrade-runtime Chore/upgrade runtime Aug 2, 2024
@jkoenig134 jkoenig134 changed the title Chore/upgrade runtime Upgrade runtime Aug 2, 2024
@jkoenig134
Copy link
Member

@Milena-Czierlinski & @britsta could you please check if everything is covered?

test/spec.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@britsta britsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions for understanding:

  • A route for changing the default RepositoryAttribute in the Connector should not be exposed, because it is a feature related to the App? So the isDefault property is set to true for the firstly created RepositoryAttribute of a given IdentityAttribute value type, but it should not be possible to change the default RepositoryAttribute for the time being with a Connector? @Milena-Czierlinski
  • Also, setting the communication language seems to be a feature more strongly related to the App, which is why there is no Connector route added for it, right? @Magnus-Kuhn

packages/sdk/src/types/attributes/ConnectorAttribute.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Milena-Czierlinski
Copy link
Contributor

A route for changing the default RepositoryAttribute in the Connector should not be exposed, because it is a feature related to the App? So the isDefault property is set to true for the firstly created RepositoryAttribute of a given IdentityAttribute value type, but it should not be possible to change the default RepositoryAttribute for the time being with a Connector? @Milena-Czierlinski

Yes, this is how it is currently planned.

@Milena-Czierlinski
Copy link
Contributor

Is there a reason, why the deletionInfo occurs in the GetOwnRepositoryAttributesRequest, but not in the GetOwnSharedIdentityAttributesRequest or GetPeerSharedIdentityAttributesRequest, for example? I would expect it to be the other way round. What can our Attribute deletion expert @Milena-Czierlinski say about this?

Your expectation is correct. In the runtime it is implemented the other way around.

@Magnus-Kuhn
Copy link
Contributor Author

Also, setting the communication language seems to be a feature more strongly related to the App, which is why there is no Connector route added for it, right?

Exactly.

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 72.24490% with 68 lines in your changes missing coverage. Please review.

Files Patch % Lines
...ypes/messages/ConnectorMessageContentDerivation.ts 0.00% 25 Missing ⚠️
.../ConnectorRelationshipCreationContentDerivation.ts 0.00% 13 Missing ⚠️
...k/src/types/notifications/ConnectorNotification.ts 0.00% 7 Missing ⚠️
packages/sdk/src/endpoints/Endpoint.ts 73.33% 4 Missing ⚠️
...c/types/notifications/ConnectorNotificationItem.ts 0.00% 4 Missing ⚠️
...ages/sdk/src/endpoints/IncomingRequestsEndpoint.ts 57.14% 3 Missing ⚠️
...sdk/src/endpoints/RelationshipTemplatesEndpoint.ts 86.66% 2 Missing ⚠️
packages/sdk/src/endpoints/TokensEndpoint.ts 77.77% 2 Missing ⚠️
packages/sdk/src/types/notifications/index.ts 0.00% 2 Missing ⚠️
packages/sdk/src/endpoints/AttributesEndpoint.ts 95.00% 1 Missing ⚠️
... and 5 more
Files Coverage Δ
packages/sdk/src/endpoints/AccountEndpoint.ts 100.00% <100.00%> (ø)
packages/sdk/src/endpoints/ChallengesEndpoint.ts 100.00% <100.00%> (ø)
...ges/sdk/src/types/attributes/ConnectorAttribute.ts 100.00% <100.00%> (ø)
packages/sdk/src/types/index.ts 100.00% <100.00%> (ø)
...ackages/sdk/src/types/messages/ConnectorMessage.ts 100.00% <100.00%> (ø)
.../types/messages/ConnectorMessageWithAttachments.ts 100.00% <100.00%> (ø)
.../src/types/messages/requests/SendMessageRequest.ts 100.00% <100.00%> (ø)
...tionshipTemplates/ConnectorRelationshipTemplate.ts 100.00% <100.00%> (ø)
.../ConnectorRelationshipTemplateContentDerivation.ts 100.00% <100.00%> (ø)
...kages/sdk/src/types/relationshipTemplates/index.ts 100.00% <100.00%> (ø)
... and 24 more

jkoenig134
jkoenig134 previously approved these changes Aug 14, 2024
Copy link
Member

@jkoenig134 jkoenig134 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But wait for other reviews.

@jkoenig134 jkoenig134 merged commit 1738600 into release/v5 Aug 15, 2024
9 checks passed
@jkoenig134 jkoenig134 deleted the chore/upgrade-runtime branch August 15, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update dependencies runtime-upgrade Pull requests that update the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants