kontent-ai / data-ops

The Official CLI Tool for manipulating data in Kontent.ai
MIT License
13 stars 2 forks source link

Support secure assets #100

Open andgdk opened 6 days ago

andgdk commented 6 days ago

Motivation

Currently, for environments with secure assets delivery enabled, the resulting backup contains assets with a file size of 0 kB in the assets/ folder.

This is critical as for certain users, the backup is incomplete, and no error/warning is provided.

Preposition Support could be added by providing the bearer token to the fetching of assets, I suppose: https://github.com/kontent-ai/data-ops/blob/9da88a5618749d31e88264297da2b27f700ca1fe/src/modules/backupRestore/backupRestoreEntities/entities/assets.ts#L74

I am in general wondering if the support for secure assets would better be implemented into another package. Also, I did not fully comprehend which places else or which modules would be affected (e.g. migrations?).

I will attach a PR to highlight my preposition. The way I handled the options is not very elegant, I appreciate suggestions to improve. In case you accept contributions on this request, I would be happy to collaborate.

There might be blind spots in the existing tests regardless of using secure assets or not, which I did not fully evaluate: This is apparently because only *.json files of the backup are tested. The tests never assert the existence or size of the assets/ folder or its contents, as far as I can tell.

Proposed solution

Investigate the impact of adding support for secure assets, considering if this should be a core feature, if other @kontent-ai packages would benefit. Simplest:

Notes about tests, regardless of secure assets: Assertions for asset files appear to be missing; Assert size of assets folder or better individual file size within assets folder or - even better - compare bitwise.

Additional context

IvanKiral commented 5 days ago

Hey @andgdk,

thank you for the detailed issue (I appreciate it, it's the best I have seen so far :)) and the PR. As this is not a common scenario for me, I need to investigate a solution slightly. Is this time-dependent for you?

It is very likely that for now, we will stick with the "workaround solution" you provided. I already had a quick look at the PR and it looked good I will probably have maybe some refactoring and naming suggestions :)

as for other modules, I think the other one would be the migration-toolkit repo (utilized by migrate-content in data-ops)

andgdk commented 5 days ago

Thanks for your prompt reply and kind words @IvanKiral :)

It's not time-critical as I have this workaround and in worst case another implementation with the deprecated backup-manager.

Feel free to make any adaptations or start from a blank sheet - what I did was not elegant at all and I couldn't run the provided integration tests. But now you are with me on this case.

I hope to come around until next week to have a look at the structure of the migration-toolkit to get familiar. Also I may try to write some test cases as this would be a good practice for me.