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

feat(cli): Add existing assets to album and allow album name #5838

Merged
merged 129 commits into from
Dec 20, 2023

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Dec 18, 2023

  • Add assets to existing albums without uploading
  • Allow user to specify album name

Thanks to the work by @Willena and adapted from #5843 and #5861

Added e2e tests for these scenarios.

Copy link

cloudflare-pages bot commented Dec 18, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 317b41e
Status: ✅  Deploy successful!
Preview URL: https://1b9c8dfb.immich.pages.dev
Branch Preview URL: https://feat-cli-add-to-album.immich.pages.dev

View logs

@etnoy etnoy added the cli Tasks related to the Immich CLI label Dec 18, 2023
@Willena
Copy link
Contributor

Willena commented Dec 19, 2023

Just my thoughts here.
We should keep the hash check in any situation (unless explicitly disabled by the user). It makes no sense to re-upload the same asset if in the end all we want is adding the asset to an album.

Adding to album should be independent from uploading the asset.
We could be using the checkBulkUpload response to determine the assetId in case it is a duplicate.

(FYI I was about to implement this, but I saw your PR)

@etnoy
Copy link
Contributor Author

etnoy commented Dec 19, 2023

Just my thoughts here. We should keep the hash check in any situation (unless explicitly disabled by the user). It makes no sense to re-upload the same asset if in the end all we want is adding the asset to an album.

Adding to album should be independent from uploading the asset. We could be using the checkBulkUpload response to determine the assetId in case it is a duplicate.

(FYI I was about to implement this, but I saw your PR)

Thanks for your feedback, maybe I will take another look at this and implement a separate code path for only adding to the album.

@Willena
Copy link
Contributor

Willena commented Dec 19, 2023

@etnoy PR #5861 implements my previous comment.

@etnoy
Copy link
Contributor Author

etnoy commented Dec 19, 2023

@etnoy PR #5861 implements my previous comment.

Thanks, I'm currently writing code, too. Unfortunately we can no longer use the old deviceasset id checks as they are not reliable and will be deprecated. After some testing we likely have to upload assets every time in order to match between client and server in a reliable way.

@Willena
Copy link
Contributor

Willena commented Dec 19, 2023

Unfortunately we can no longer use the old deviceasset id checks as they are not reliable and will be deprecated

I'm not sure to understand: the bulk-upload-check api is checking the presence of the asset by comparing the sha1 computed on the server with a sha1 that the cli provides.

getAssetsByChecksums(ownerId: string, checksums: Buffer[]): Promise<AssetCheck[]> {
return this.assetRepository.find({
select: {
id: true,
checksum: true,
},
where: {
ownerId,
checksum: In(checksums),
},
withDeleted: true,
});
}

It does not seeam to be using the deviceAssetId to make the comparison;

The upload api is using the same method to detect a duplicate.

const [duplicate] = await this._assetRepository.getAssetsByChecksums(auth.user.id, checksums);

Each time the returned asset id correspond to the uuid generated for that asset the first time it got uploaded; This uuid is independant of the deviceAssetId.

@etnoy
Copy link
Contributor Author

etnoy commented Dec 19, 2023

Unfortunately we can no longer use the old deviceasset id checks as they are not reliable and will be deprecated

I'm not sure to understand: the bulk-upload-check api is checking the presence of the asset by comparing the sha1 computed on the server with a sha1 that the cli provides.

getAssetsByChecksums(ownerId: string, checksums: Buffer[]): Promise<AssetCheck[]> {
return this.assetRepository.find({
select: {
id: true,
checksum: true,
},
where: {
ownerId,
checksum: In(checksums),
},
withDeleted: true,
});
}

It does not seeam to be using the deviceAssetId to make the comparison;
The upload api is using the same method to detect a duplicate.

const [duplicate] = await this._assetRepository.getAssetsByChecksums(auth.user.id, checksums);

Each time the returned asset id correspond to the uuid generated for that asset the first time it got uploaded; This uuid is independant of the deviceAssetId.

You are right, my mistake. I misunderstood before and your implementation is correct as it does not use deviceAssetId, only hash. Can I adapt the implementation from your PR into this one, merging all album-related tasks into one PR?

@Willena
Copy link
Contributor

Willena commented Dec 19, 2023

You are right, my mistake.

No worries :)

Can I adapt the implementation from your PR into this one, merging all album-related tasks into one PR?

Yes, of course !

@etnoy etnoy changed the title feat(cli): Don't hash when adding to album feat(cli): Add existing assets to album and allow album name Dec 19, 2023
cli/src/commands/upload.ts Outdated Show resolved Hide resolved
cli/src/commands/upload.ts Outdated Show resolved Hide resolved
@etnoy
Copy link
Contributor Author

etnoy commented Dec 19, 2023

All done, should be ready to merge

@alextran1502 alextran1502 merged commit 8295542 into main Dec 20, 2023
20 checks passed
@alextran1502 alextran1502 deleted the feat/cli-add-to-album branch December 20, 2023 15:51
martabal pushed a commit that referenced this pull request Jan 9, 2024
* Allow building and installing cli

* feat: add format fix

* docs: remove cli folder

* feat: use immich scoped package

* feat: rewrite cli readme

* docs: add info on running without building

* cleanup

* chore: remove import functionality from cli

* feat: add logout to cli

* docs: add todo for file format from server

* docs: add compilation step to cli

* fix: success message spacing

* feat: can create albums

* fix: add check step to cli

* fix: typos

* feat: pull file formats from server

* chore: use crawl service from server

* chore: fix lint

* docs: add cli documentation

* chore: rename ignore pattern

* chore: add version number to cli

* feat: use sdk

* fix: cleanup

* feat: album name on windows

* chore: remove skipped asset field

* feat: add more info to server-info command

* chore: cleanup

* wip

* chore: remove unneeded packages

* e2e test can start

* git ignore for geocode in cli

* add cli e2e to github actions

* can do e2e tests in the cli

* simplify e2e test

* cleanup

* set matrix strategy in workflow

* run npm ci in server

* choose different working directory

* check out submodules too

* increase test timeout

* set node version

* cli docker e2e tests

* fix cli docker file

* run cli e2e in correct folder

* set docker context

* correct docker build

* remove cli from dockerignore

* chore: fix docs links

* feat: add cli v2 milestone

* fix: set correct cli date

* remove submodule

* chore: add npmignore

* chore(cli): push to npm

* fix: server e2e

* run npm ci in server

* remove state from e2e

* run npm ci in server

* reshuffle docker compose files

* use new e2e composes in makefile

* increase test timeout to 10 minutes

* make github actions run makefile e2e tests

* cleanup github test names

* assert on server version

* chore: split cli e2e tests into one file per command

* chore: set cli release working dir

* chore: add repo url to npmjs

* chore: bump node setup to v4

* chore: normalize the github url

* check e2e code in lint

* fix lint

* test key login flow

* feat: allow configurable config dir

* fix session service tests

* create missing dir

* cleanup

* bump cli version to 2.0.4

* remove form-data

* feat: allow single files as argument

* add version option

* bump dependencies

* fix lint

* wip use axios as upload

* version bump

* cApiTALiZaTiON

* don't touch package lock

* wip: don't use job queues

* don't use make for cli e2e

* fix server e2e

* feat: always skip hashing when adding albums

* feat: create album with specific name

* check asset duplication before adding to album

* update documentation

* use correct check for when to create albums

---------

Co-authored-by: Alex <alex.tran1502@gmail.com>
Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Tasks related to the Immich CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants