Skip to content

Conversation

eric-pSAP
Copy link
Contributor

@eric-pSAP eric-pSAP commented Sep 30, 2025

This is meant to add more information as well as clarify current information. Also want to align with the README of the Java version of Attachments. Steps remaining:

  • Finalize structure and sections
    • Removing Java-specific sections and information
  • Confirm and clarify details
  • Add in links to sources

This initial change should make the rest of the editing process more streamlined

@eric-pSAP eric-pSAP self-assigned this Sep 30, 2025
@eric-pSAP eric-pSAP added documentation Improvements or additions to documentation good first issue Good for newcomers labels Sep 30, 2025
@KoblerS KoblerS self-requested a review October 7, 2025 14:21
README.md Outdated
Comment on lines 344 to 355
The unit tests in this module do not need a binding to the respective object stores, run them with `mvn clean install`.
The integration tests need a binding to a real object store. Run them with `mvn clean install -Pintegration-tests-oss`.
To set the binding, provide the following environment variables:
- AWS_S3_BUCKET
- AWS_S3_REGION
- AWS_S3_ACCESS_KEY_ID
- AWS_S3_SECRET_ACCESS_KEY
##### Implementation details
This artifact provides custom handlers for events from the [AttachmentService](../../cds-feature-attachments/src/main/java/com/sap/cds/feature/attachments/service/AttachmentService.java).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adapt to Jest

README.md Outdated
logging:
level:
...
'[com.sap.cds.feature.attachments]': DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if it is called com.sap.cds.feature.attachments or com.sap.cds.attachments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we find this out? This note comes from Java

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume it's called attachments

eric-pSAP and others added 8 commits October 8, 2025 10:40
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>

### Package Setup

The attachments plugin needs to be referenced in the package.json of the consuming CAP NodeJS application:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this in capire docs, but this is not how we are using the application. Which to use? https://cap.cloud.sap/docs/node.js/cds-plugins

Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably can ignore what's written in the cap docs as it does not really cover this use case

Copy link
Collaborator

Choose a reason for hiding this comment

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

you might want to add an example how the profile should look like, in case if I didn't missed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by profile?

README.md Outdated
1. Log in to Cloud Foundry:

```sh
cf login -a <CF-API> -o <ORG-NAME> -s <SPACE-NAME>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rupa has these two comments on this line as well: "Consider linking to the setup page of SAP Object Store. It may be this - https://help.sap.com/docs/object-store/object-store-service-on-sap-btp/configuring-parameters-of-object-store-instance?locale=en-US"
"You can also provide information about where someone can get CF, Org name and Space name details"

README.md Outdated

When using a dedicated storage target, the attachment is not stored in the underlying database; instead, it is saved on the specified storage target and only a reference to the file is kept in the database, as defined in the CDS model.

For using SAP Object Store, you must already have an SAP Object Store service instance with a storage target which you can access. To connect it, follow this setup.
Copy link
Contributor Author

@eric-pSAP eric-pSAP Oct 8, 2025

Choose a reason for hiding this comment

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

Should this be further synced this with the other Object Stores section below?

Copy link
Contributor Author

@eric-pSAP eric-pSAP left a comment

Choose a reason for hiding this comment

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

Transferring old notes

> [!Note]
> Starting from version 2.1.0, **separate mode** for object store instances is the default setting for multitenancy.
> As of version 2.2.0, both the `standard` and `S3-standard` plans of the SAP Object Store offering are supported.
> **Important:** The `S3-standard` plan is no longer available for new subscriptions. For new object store instances, use the `standard` plan.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note from Rupa: "Are these two lines contradicting each other?"

To ensure tenant identification when using a shared object store instance, the plugin prefixes attachment URLs with the tenant ID. Be sure the shared object store instance is bound to the `mtx` application module before deployment.
### Object Stores
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Java version, should this be combined with Object Storage section above?

README.md Outdated
The unit tests in this module do not need a binding to the respective object stores, run them with `mvn clean install`.
The integration tests need a binding to a real object store. Run them with `mvn clean install -Pintegration-tests-oss`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to update this line, is the -Pintegration-tests-oss part the same for Node?

Co-authored-by: Simon Kobler <[email protected]>
README.md Outdated
The unit tests in this module do not need a binding to the respective object stores, run them with `npm install`. To achieve a clean install, the comman `rm -rf node_modules` should be used before installation.
The integration tests need a binding to a real object store. Run them with `npm install -Pintegration-tests-oss`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-Pintegration-tests-oss is from Java, needed for Node?

Comment on lines +40 to 42
- The plugin is self-configuring as described in [Package Setup](#package-setup). To enable attachments, simply add the plugin package to your project:
```sh
npm add @cap-js/attachments
Copy link
Collaborator

Choose a reason for hiding this comment

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

as I'm not really sure, more of a question: does npm add, add the config to the package.json like to use which cloud provider in which profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not believe so but we can check. From what I know, it simply adds the plugin.


### Package Setup

The attachments plugin needs to be referenced in the package.json of the consuming CAP NodeJS application:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably can ignore what's written in the cap docs as it does not really cover this use case


### Package Setup

The attachments plugin needs to be referenced in the package.json of the consuming CAP NodeJS application:
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might want to add an example how the profile should look like, in case if I didn't missed it

README.md Outdated
logging:
level:
...
'[com.sap.cds.feature.attachments]': DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume it's called attachments

eric-pSAP and others added 8 commits October 8, 2025 16:27
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
Co-authored-by: Simon Kobler <[email protected]>
By default, malware scanning is enabled for all profiles unless no storage provider has been specified. You can configure malware scanning by setting:
```json
"attachments": {
"scan": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not either, I assumed it was already true

@eric-pSAP eric-pSAP requested a review from lisajulia October 8, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants