feat: make versions work with id

[nassiharel]

This change is 

[nassiharel]

/deploy

[yehiyam]

Reviewed 25 of 25 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @nassiharel)

---

core/api-server/api/rest-api/app-server.js, line 69 at r1 (raw file):

🚀

really? :-)

---

core/api-server/api/rest-api/routes/v1/versions.js, line 21 at r1 (raw file):

id

id is the version or the tag?

---

core/api-server/api/rest-api/routes/v1/versions.js, line 37 at r1 (raw file):

    router.delete('/algorithms/:name/:id', logger(), async (req, res, next) => {
        const { id, name } = req.params;
        const response = await versionsService.deleteVersion({ id, name });

Is this route used to delete version or algorithm?

---

core/api-server/lib/service/algorithms.js, line 35 at r1 (raw file):

algorithmVersion

should be algorithmVersions

---

core/api-server/lib/service/algorithms.js, line 278 at r1 (raw file):

algorithm.options.debug

I don't think we need a version for debug.

---

core/api-server/lib/service/algorithms.js, line 278 at r1 (raw file):

!buildId

why not when there is a build? the version is created after the build is done?

---

core/api-server/lib/service/builds.js, line 183 at r1 (raw file):

    _formatDiff(algorithm) {
        const { fileInfo, env, gitRepository } = algorithm;

maybe add baseImage also?

---

core/api-server/tests/algorithms-store.js, line 1692 at r1 (raw file):

                const res3 = await request(req3);
                const res4 = await request(req4);
                const versions1 = res4.body.map(v => v.id).sort();

why do you need sort? versions are not returned in the order they were created?

---

core/api-server/tests/versions.js, line 295 at r1 (raw file):

            expect(res.body.error.message).to.equal(`version no-such Not Found`);
        });
        it('should succeed to tag version', async () => {

should add tests to delete tags and to change tags

[nassiharel]

Reviewable status: 11 of 36 files reviewed, 9 unresolved discussions (waiting on @yehiyam)

---

core/api-server/api/rest-api/app-server.js, line 69 at r1 (raw file):

Previously, yehiyam wrote…

🚀

really? :-)

:-)

---

core/api-server/api/rest-api/routes/v1/versions.js, line 21 at r1 (raw file):

Previously, yehiyam wrote…

id

id is the version or the tag?

version (uid length of 10)

---

core/api-server/api/rest-api/routes/v1/versions.js, line 37 at r1 (raw file):

Previously, yehiyam wrote…

Is this route used to delete version or algorithm?

version

---

core/api-server/lib/service/algorithms.js, line 35 at r1 (raw file):

Previously, yehiyam wrote…

algorithmVersion

should be algorithmVersions

Done.

---

core/api-server/lib/service/algorithms.js, line 278 at r1 (raw file):

Previously, yehiyam wrote…

algorithm.options.debug

I don't think we need a version for debug.

Done.

---

core/api-server/lib/service/algorithms.js, line 278 at r1 (raw file):

Previously, yehiyam wrote…

!buildId

why not when there is a build? the version is created after the build is done?

Yes

---

core/api-server/lib/service/builds.js, line 183 at r1 (raw file):

Previously, yehiyam wrote…

maybe add baseImage also?

Done.

---

core/api-server/tests/algorithms-store.js, line 1692 at r1 (raw file):

Previously, yehiyam wrote…

why do you need sort? versions are not returned in the order they were created?

Yes, but desc... Done

---

core/api-server/tests/versions.js, line 295 at r1 (raw file):

Previously, yehiyam wrote…

should add tests to delete tags and to change tags

Done.

[yehiyam]

Reviewed 26 of 26 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nassiharel)

---

core/api-server/api/rest-api/swagger/definitions/algorithmApplyOptions.yaml, line 5 at r2 (raw file):

should replace the current image with this image

should set the newly created version as current

---

core/api-server/lib/service/versions.js, line 86 at r2 (raw file):

versions && versions[0] && versions[0].version

versions?.[0]?.versions

[nassiharel]

Reviewable status: 32 of 37 files reviewed, 2 unresolved discussions (waiting on @yehiyam)

---

core/api-server/api/rest-api/swagger/definitions/algorithmApplyOptions.yaml, line 5 at r2 (raw file):

Previously, yehiyam wrote…

should replace the current image with this image

should set the newly created version as current

Done.

---

core/api-server/lib/service/versions.js, line 86 at r2 (raw file):

Previously, yehiyam wrote…

versions && versions[0] && versions[0].version

versions?.[0]?.versions

Done.

[nassiharel]

/deploy

[yehiyam]

/deploy

[nassiharel]

/deploy

[yehiyam]

/deploy

[yehiyam]

Reviewed 6 of 7 files at r3, 2 of 2 files at r4.
Reviewable status: 38 of 39 files reviewed, all discussions resolved (waiting on @yehiyam)

[yehiyam]

Reviewed 1 of 7 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[yehiyam]

Reviewed 3 of 4 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nassiharel)

---

core/api-server/lib/service/versions.js, line 98 at r5 (raw file):

        try {
            semver = await this._lockSemver(name, semver);

what happens if not successful 3 times?

---

core/api-server/lib/service/versions.js, line 118 at r5 (raw file):

    }

    async _lockSemver(name, semver) {

perhaps put this method in stateManager?

[nassiharel]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yehiyam)

---

core/api-server/lib/service/versions.js, line 98 at r5 (raw file):

Previously, yehiyam wrote…

what happens if not successful 3 times?

It gets the latest incremented semver.
If you will run the test should create multiple versions in concurrent mode you will see that happens.
The test simulate 4 parallel requests, and gets ['1.0.3', '1.0.2', '1.0.1', '1.0.0']
If I will run 5 parallel requests I will get ['1.0.3', '1.0.3', '1.0.2', '1.0.1', '1.0.0'] which is not so good.

So I can change it to max 5 attempts, anyway, we have a rate-limiter of 5 requests...

---

core/api-server/lib/service/versions.js, line 118 at r5 (raw file):

Previously, yehiyam wrote…

perhaps put this method in stateManager?

This method call inside to _incSemver(); (I can use cb)
But anyway this is a specific logic to versions.

[nassiharel]

Reviewable status: 36 of 38 files reviewed, 2 unresolved discussions (waiting on @yehiyam)

---

core/api-server/lib/service/versions.js, line 98 at r5 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

It gets the latest incremented semver.
If you will run the test should create multiple versions in concurrent mode you will see that happens.
The test simulate 4 parallel requests, and gets ['1.0.3', '1.0.2', '1.0.1', '1.0.0']
If I will run 5 parallel requests I will get ['1.0.3', '1.0.3', '1.0.2', '1.0.1', '1.0.0'] which is not so good.

So I can change it to max 5 attempts, anyway, we have a rate-limiter of 5 requests...

Done.

---

core/api-server/lib/service/versions.js, line 118 at r5 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

This method call inside to _incSemver(); (I can use cb)
But anyway this is a specific logic to versions.

Done.

[yehiyam]

Reviewed 2 of 2 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved