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

Expose the copy package operation through REST API #1258

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

an0rak-dev
Copy link
Contributor

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

In case we don't have the possibility to use the aptly CLI on a repository, expose the aptly copy command through the REST API.

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@neolynx neolynx self-assigned this Apr 10, 2024
@neolynx
Copy link
Member

neolynx commented Apr 10, 2024

good idea !
are you working on the implementation ?

@an0rak-dev
Copy link
Contributor Author

good idea ! are you working on the implementation ?

@neolynx : Absolutely ! We've been a little busy last few days but we resume our work on this 😄

api/repos.go Outdated
// POST /repos/:name/copy/:src/:file
func apiReposCopyPackage(c *gin.Context) {
dstRepoName := c.Request.URL.Query().Get("name")
srcRepoName := c.Request.URL.Query().Get("name")
Copy link
Member

Choose a reason for hiding this comment

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

name twice ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah and this was a bad idea 😅, we went with c.Params.ByName(key) which is actually working. I did a few manual tests which seem to indicate this work. To complete this and cover any future modification of this API, @an0rak-dev will propose a series of tests that goes with this API.

Copy link
Member

Choose a reason for hiding this comment

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

awesome ! thanks for your work, guys 👍

@neolynx neolynx added needs review Ready for review & merge and removed needs implementation WIP labels Jun 8, 2024
@neolynx neolynx self-requested a review June 15, 2024 23:08
@neolynx neolynx added the increase coverage The PR lacks test coverage label Jun 15, 2024
@an0rak-dev
Copy link
Contributor Author

@neolynx I have a dilemma regarding the tests of the new /repos copy endpoint.

I'll need to create a local aptly repo with an existing .deb package in it while setting up the test. To do so, I have three possibilities :

  1. We curl a light .deb from a well known public mirror like Debian (pro : easy to implement and platform independent. Cons : unit testing requires an active internet connection and you'll end with a random .deb file on your local system).
  2. We embedded a prebuilt, mock .deb file in the git repo as a test resource and we copy it to setup our tests (pro : no internet connection required, all the tooling to run the test is already present in the repo. Cons : We add a random binary package to the git repo.)
  3. We set up a mock executable with the debian folder and all the needed files (control,copyright, rules...) and in the make test target we forge the .deb locally using those before running the test. (Pro : self sufficient and no random binary package. cons : will increase the setup time of the test and will need some tweaks to run unit testing on systems which aren't debian based).

I kind of like the choice 3, and I'll say that at worst we can still forge the .deb using a local docker container with a volume.

What do you think ?

@neolynx
Copy link
Member

neolynx commented Jul 1, 2024

I think we already have option 2:

$ find -name *.deb
./deb/testdata/dbgsym-with-source-version/dbgsym-with-source-version_0.1+aptly2021.01_amd64.deb
./deb/testdata/changes/hardlink_0.2.0_i386.deb
./deb/testdata/changes/hardlink_0.2.1_amd64.deb
./system/t06_publish/PublishRepo29Test/libboost-broken-program-options-dev_1.49.0.1_i386.deb
./system/t06_publish/FSEndpointPublishSnapshot17Test/2/libboost-broken-program-options-dev_1.49.0.1_i386.deb
./system/t06_publish/FSEndpointPublishSnapshot17Test/1/libboost-broken-program-options-dev_1.49.0.1_i386.deb
./system/t06_publish/FSEndpointPublishSnapshot18Test/2/libboost-broken-program-options-dev_1.49.0.1_i386.deb
./system/t06_publish/FSEndpointPublishSnapshot18Test/1/libboost-broken-program-options-dev_1.49.0.1_i386.deb
./system/changes/hardlink_0.2.1_amd64.deb
./system/t04_mirror/test_release2/pool/main/a/amanda/amanda-client_3.3.1-3~bpo60+1_amd64.deb
./system/files/libboost-program-options-dev_1.49.0.1_i386.deb
./system/files/libboost-program-options-dev_1.62.0.1_i386.deb

and it might be easier to create a system test for the copy API, see ReposAPITestAddFile in system/t12_api/repos.py.

the system tests you should be able to prepare with make docker-build-system-tests and then run them with make docker-system-tests, optionally with TEST=t12_api (since running all tests takes ~20mins, but might depend on previous tests to be run once)

I would suggest to remove the api/repos_test.go and create a system test in python...

@an0rak-dev
Copy link
Contributor Author

Oh ! My bad, I was mislead by the existing api/*_test.go files.

I'll rewrite the test using the existing E2E system.

@neolynx
Copy link
Member

neolynx commented Jul 2, 2024

@an0rak-dev an0rak-dev force-pushed the copy_package_url branch 2 times, most recently from 996b071 to 0a304cd Compare July 3, 2024 22:01
@neolynx
Copy link
Member

neolynx commented Jul 3, 2024

thanks for your amazing work, @an0rak-dev !

seems like the test is failing:

[GIN] 2024/07/03 - 22:08:30 | 202 |   11.153459ms |       127.0.0.1 | POST     "/api/repos/FaAZ8T9QodtFhKL/copy/LztTvFCdwDRcIlw/libboost-program-options-dev_1.49.0.1_i386.deb?_async=True"
[GIN] 2024/07/03 - 22:08:30 | 200 |    5.489289ms |       127.0.0.1 | GET      "/api/tasks/38/wait"
[GIN] 2024/07/03 - 22:08:30 | 200 |   10.977309ms |       127.0.0.1 | GET      "/api/tasks/38"
[GIN] 2024/07/03 - 22:08:30 | 200 |   10.178303ms |       127.0.0.1 | GET      "/api/repos/FaAZ8T9QodtFhKL/packages"
[GIN] 2024/07/03 - 22:13:30 | 200 |    85.05132ms |       127.0.0.1 | GET      "/api/repos/FaAZ8T9QodtFhKL/packages"
FAIL
Traceback (most recent call last):
  File "/app/system/run.py", line 126, in run
    t.test()
  File "/app/system/lib.py", line 179, in test
    self.check()
  File "/app/system/t12_api/repos.py", line 393, in check
    self.check_equal(self.get(f"/api/repos/{repo2_name}/packages").json(),
  File "/app/system/lib.py", line 421, in check_equal
    self.verify_match(a, b, match_prepare=pprint.pformat)
  File "/app/system/lib.py", line 472, in verify_match
    raise Exception("content doesn't match:\n" + diff + "\n")
Exception: content doesn't match:
--- 
+++ 
@@ -1 +1 @@
-[]
+['Pi386 libboost-program-options-dev 1.49.0.1 918d2f433384e378']

the 2nd repo returns an emtpy list... was the package not copied ?

I think there should be no ".deb" in the :file argument...

@an0rak-dev an0rak-dev marked this pull request as ready for review July 9, 2024 17:05
Copy link
Member

@neolynx neolynx left a comment

Choose a reason for hiding this comment

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

great, thanks !

@neolynx neolynx merged commit 11401ca into aptly-dev:master Jul 10, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
increase coverage The PR lacks test coverage needs review Ready for review & merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants