Add deb repository and remote pages by warisshaikh1 · Pull Request #347 · pulp/pulp-ui

warisshaikh1 · 2026-08-28T18:16:13Z

Addresses part of #277 (deb in the side menu).

Draft on purpose — I asked three scoping questions in #277 and haven't heard back yet. This is here so there's something concrete to react to rather than to pre-empt the answers. Happy to cut it down, extend it, or restructure it entirely.

What this adds

A Pulp deb menu section gated on hasPlugin('deb'), with Repositories and Remotes, mirroring Pulp file:

	list	detail	edit	tabs
`remotes/deb/apt`	yes	yes	yes	details
`repositories/deb/apt`	yes	yes	yes	details, versions, distributions

What it deliberately leaves out

Publications. deb has two publication endpoints rather than a field: publications/deb/apt generates fresh metadata and needs a signing service to be consumable, while publications/deb/verbatim republishes upstream's Release/InRelease byte for byte so an ordinary ubuntu-archive-keyring verifies it. A publications tab has to either show both or ask which to create, which is a design decision rather than wiring, so it seemed better to agree it first.
Content browsing. content/deb/packages is the useful one, but there are a dozen deb content endpoints where rpm has one. Easy follow-up once the shape here is agreed.

The one place deb genuinely differs from file

RemoteForm needed the APT fields, which have no equivalent elsewhere: distributions (suites), components, architectures, gpgkey, and the sync_sources / sync_udebs / sync_installer switches.

distributions is required, for deb only. Every other plugin's remote needs just a URL; pulp_deb rejects one without suites:

POST /pulp/api/v3/remotes/deb/apt/
{"name":"x","url":"http://deb.debian.org/debian/","tls_validation":true,"download_concurrency":10}
-> 400 {"distributions": ["This field is required."]}

So requiredFields becomes conditional. gpgkey reuses the FileUpload treatment ca_cert and client_cert already get, since it's an armoured key file rather than a one-line value.

Everything else is registry entries — plugin2api, the plugin unions on LazyRepositories / LazyDistributions / RepositoryForm, and deb-only optional fields on the shared RemoteType. All additive; ansible, container and file behaviour is unchanged (the RemoteForm diff is worth a look on that point specifically).

Testing

Verified against a live pulpcore 3.116.0 / pulp_deb 3.10.0 mirroring Ubuntu noble, with the dev server proxied at it (API_PROXY):

every query the new pages issue returns 200 — remote and repository lists with ordering / name__icontains / pulp_label_select / remote filters, …/versions/, …/versions/?number=N, and distributions/deb/apt/?repository=<href>
the exact create payload DebRemoteEdit sends → 201; the smartUpdate PUT with all APT fields → 202, and the fields read back verbatim
the repository create plus auto-created distribution path → 201 / 202, and pluginRepositoryBasePath('deb', …) resolves the base_path the Repository URL field shows
test objects cleaned up afterwards, existing content untouched

npm run lint:js, lint:ts, lint:ls, the this-without-class check and npm run build are all clean (build warnings unchanged at 18, all pre-existing bundle-size ones). No CHANGES.md entry, since that's generated from PR titles at release.

Two things I'd flag for review

A third copy of SyncModal. ansible-repository-sync.tsx and file-repository-sync.tsx already carry identical copies and I've followed suit rather than refactoring inside a feature PR. Happy to extract all three into a shared component, here or separately, if you'd prefer.
mirror defaults to true in the sync modal, matching the other plugins, though pulp_deb's own API default is false. I kept UI consistency, but say the word and I'll flip it — mirroring is the destructive direction.

@bmbouter @himdel @dkliban @ipanova

Redtigercod4

Thanks for this - solid work overall, and I've left comments on the specific issues, tagged using MoSCoW. A couple of things I'd suggest:

Commit size: This is a large PR, 1,942 lines across 32 files submitted as a single commit. That makes it much harder for reviewers to follow, especially when trying to understand how the implementation came together. Smaller, sequential PRs would be preferred going forward.
Cypress Testing: As this is new UI functionality within PulpUI, could you please add corresponding test cases to cypress/e2e/smoke.js.
AI-assisted contributions: No issue either way, but if any AI tooling was used, please make sure the appropriate attributions is included, in line with Pulp's AI policy.

Ref:
https://github.com/pulp/.github/blob/cf562eff2ec6fe0f49bdfd693c3627453eef5203/profile/README.md

Redtigercod4 · 2026-09-13T13:49:17Z

+  { name, pulp_href, pulpId },
+  { addAlert, setState, listQuery },
+) {
+  // TODO: handle more pages


MUST: Bare // TODO isn't enough here - if the repository being deleted has more than 100 distributions, the excess are silently orphaned. I'm aware this same pattern exists in both ansible and file repository actions, but I don't want to carry it over into deb as well.

Please either link this TODO to a tracked Github issue, or implement pagination using the offset value returned by the API.

Implemented the pagination. listDistributions() walks pages until it has collected as many as count reports, and breaks early on an empty page so a count that disagrees with reality cannot spin the loop.

881e49e

Redtigercod4 · 2026-09-13T14:01:27Z

@@ -0,0 +1,149 @@
+import { msg, t } from '@lingui/core/macro';


MUST (mirror default): Good to see this flagged on the PR already - I'd agree it's worth resolving to match what pulp_deb defaults to, since that's the safer, non-destruction option. Aware ansible and file hardcode true too, but I'll raise a separate issue to track.

SHOULD (SyncModal duplication): Also flagged by you already - agreed this is worth doing, but I'd suggest pulling SyncModal out into a shared component as a precursor PR rather than here, since it's identical to file and ansible.

Flipped to mirror: false. It was written out in three places, so it is one DEFAULT_SYNC_PARAMS constant now, with a note there on why deb does not follow ansible and file.

On the SyncModal duplication: agreed, and agreed it belongs in its own PR rather than this one. I will follow up with one that pulls it out for ansible and file, and drop this copy once that lands.

2f2e296

Opened #363 for the extraction. Once that lands I will rebase this branch on it and drop the copy here.

Redtigercod4 · 2026-09-13T14:09:06Z

+  listUrl: formatPath(Paths.deb.remote.list),
+  query: ({ name }) =>
+    DebRemoteAPI.list({ name })
+      .then(({ data: { results } }) => results[0])


MUST: results[0] assumes an array with no local .catch - if the response shape is ever wrong, it throws past page-with-tabs.tsx catch handler, and the page would hang silently. Also, the comment here is a little misleading, worth rewording so it's clear we're synthesizing a not-found rejection, not describing actual API behaviour.

Both detail pages read data?.results?.[0] now and reject explicitly, so nothing throws past the catch. Reworded the comment to say what is actually happening: the list endpoint answers 200 with an empty list, and it is this code that synthesizes the 404. Did the repository page in the same commit since it had the same shape.

728eced

Redtigercod4 · 2026-09-13T14:12:51Z

-      : plugin === 'file'
-        ? FileRemoteAPI.list({ ...(name ? { name__icontains: name } : {}) })
-        : Promise.reject(plugin)
+      : plugin === 'deb'


SHOULD: Nested ternary here is easy to misread and easy to break silently if a fourth plugin is added. Suggest breaking it out into a helper function with a switch statement instead.

Dropped the chain. plugin2api already maps a plugin to its repository and distribution APIs, so I added RemoteAPI to it and the form asks for that -- one switch to extend when a plugin arrives, rather than a conditional to unpick.

a14567c

Redtigercod4 · 2026-09-13T14:44:21Z

+        addAlert,
+        query,
+        hasPermission,
+        hasObjectPermission: (_p: string): boolean => true,


SHOULD: hasObjectPermission here always returns true, unlike the ansible implementation which conducts a real check against item.my_permissions. It's most likely harmless today, but it's a potential landmine for whoever wires up a permission-gated action later and copies in this stub.

Took it out rather than filling it in.

Checked against a live pulp_deb 3.10: repositories/deb/apt/ does not return my_permissions (nor does repositories/file/file/), so the ansible-style check would answer no to everyone here -- a quieter landmine than the stub. Nothing in this list is gated on a permission, so the field is gone and there is a comment in its place saying that an action added later wants a real check.

b3a2811

Redtigercod4 · 2026-09-13T14:48:09Z

@@ -0,0 +1,71 @@
+import { PulpAPI } from './pulp';
+
+export class DebRemoteType {


WOULD: This class only holds property definitions - no constructor or methods, never instantiated. Per TypeScript's own docs, this fits an interface better.

Converted to an interface.

15cd87f

Redtigercod4 · 2026-09-13T14:48:23Z

@@ -0,0 +1,42 @@
+import { PulpAPI } from './pulp';
+
+export class DebRepositoryType {


WOULD: This class only holds property definitions - no constructor or methods, never instantiated. Per TypeScript's own docs, this fits an interface better.

Converted to an interface.

15cd87f

Redtigercod4 · 2026-09-13T14:49:36Z

+      remote[field] = null;
+    }
+
+    // API returns headers:null bull doesn't accept it .. and we don't edit headers


COULD: Typo - bull should be but.

Fixed.

15cd87f

Redtigercod4 · 2026-09-13T14:52:15Z

+const DebRepositoryDetail = PageWithTabs<
+  DebRepositoryType & { remote?: DebRemoteType }
+>({
+  breadcrumbs: ({ name, tab, params: { repositoryVersion } }) =>


WOULD: The three tab === 'repositoru-versions' tenaries repeat the same guard to build what's really one decision - harder to read than it needs to be. Suggest collapsing into a single guarded branch instead.

Collapsed into a single guarded branch.

c4fa93c

Adds a "Pulp deb" menu section with Repositories and Remotes, mirroring the structure of Pulp file: list, detail and edit for each, plus the versions and distributions tabs on a repository. Deliberately scoped to repositories and remotes. Publications are left out because deb has two publication endpoints rather than a field -- publications/deb/apt generates fresh metadata and needs a signing service, publications/deb/verbatim republishes upstream's Release byte for byte -- so a publications tab needs a design decision rather than just wiring. Content browsing is left out for the same reason: deb has a dozen content endpoints where rpm has one. RemoteForm gains the APT fields, which have no equivalent in the other plugins: distributions (suites), components, architectures, gpgkey, and the sync_sources/sync_udebs/sync_installer switches. `distributions` is added to requiredFields for deb only -- pulp_deb answers a remote without it with "This field is required.", unlike every other plugin where url alone is enough. gpgkey reuses the FileUpload treatment the certificate fields already use, since it is an armoured key file. The remaining changes are registry entries: plugin2api, the plugin unions on LazyRepositories/LazyDistributions/RepositoryForm, and the deb-only fields on the shared RemoteType. All are additive; ansible, container and file behaviour is unchanged. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

The lookup asked for the first hundred and left a TODO for the rest, so deleting a repository with more distributions than that would leave the excess pointing at a repository that no longer exists. Walk the pages until as many have been collected as `count` reports, stopping early on an empty page so a disagreeing count cannot loop. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Opening the sync modal offered to mirror, which deletes local content the remote no longer has. The API's own default is the non-destructive one and the modal should agree with it, so the deletion is opted into. The value was written out three times; it is now one constant, which is also where the divergence from ansible and file is explained. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Both detail pages read results[0] straight out of the response, which throws on any unexpected shape, and past page-with-tabs' catch that leaves the page loading with nothing to show. Read the first result defensively instead. The comment claimed the API returns a 404; it returns 200 and an empty list, and it is this code that synthesizes the 404. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Three entries tested the same tab and were filtered back out again when the answer was no. It is one decision -- which crumbs the versions tab adds -- so make it once. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Three chained ternaries picked the remote API by plugin, repeating the same list call in each branch. plugin2api already maps a plugin to its repository and distribution APIs, so add the remote API there and let the form ask for it: one switch to extend when a plugin is added, not a conditional to unpick. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

The stub answered true to every permission. Nothing in this list is gated on one, and an apt repository carries no my_permissions field for the ansible-style check to read, so copying that check would answer no to everyone instead. Leave the permission out and say why, so an action added later gets a real check rather than a stub that always agrees. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Both types hold property declarations only, with no constructor, no methods and no instantiation, which is what an interface is for. Also fixes a typo in the smartUpdate comment. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Reaches both lists and their empty states, then opens the remote form to check the APT-only fields render, since those are what the shared form gained for this plugin. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

warisshaikh1 · 2026-09-15T10:36:39Z

Thanks for the review -- all nine comments are addressed, force-pushed and rebased on current main. Details are in the individual threads; the three broader points:

Cypress. Added Deb repositories and Deb remotes to cypress/e2e/smoke.js. Both reach the list and its empty state, and the remotes case clicks through to the create form and checks the APT-only fields render, since those are what the shared RemoteForm gained here. Green in CI.

Commit size. Point taken. I have not retro-split the original commit, since the pages only work once the API layer, routes and menu entry are all present, but the review fixes are eight separate commits, one per point, so the diffs should be easy to follow. Anything further I send will be scoped smaller from the start -- starting with the SyncModal extraction, as its own PR.

AI-assisted. Yes, AI-assisted throughout, and thanks for pointing at the policy. Every commit now carries an Assisted By: Cursor (Claude Opus 5) trailer, including the original, which I amended. Everything here has been reviewed by me, and the deb pages are running against a live Pulp with a mirrored Ubuntu archive behind them, which is where the required-field and duplicate-path behaviours in the original commit message came from.

warisshaikh1 marked this pull request as ready for review September 1, 2026 09:51

Redtigercod4 suggested changes Sep 13, 2026

View reviewed changes

warisshaikh1 force-pushed the deb-repositories-and-remotes branch from 019b5a2 to 9772856 Compare September 15, 2026 10:30

warisshaikh1 added 8 commits September 15, 2026 06:34

Collapse the deb repository breadcrumb branches

c4fa93c

Three entries tested the same tab and were filtered back out again when the answer was no. It is one decision -- which crumbs the versions tab adds -- so make it once. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Describe the deb remote and repository as interfaces

15cd87f

Both types hold property declarations only, with no constructor, no methods and no instantiation, which is what an interface is for. Also fixes a typo in the smartUpdate comment. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

Add smoke tests for the deb pages

9ce27a7

Reaches both lists and their empty states, then opens the remote form to check the APT-only fields render, since those are what the shared form gained for this plugin. Refs pulp#277 Assisted By: Cursor (Claude Opus 5)

warisshaikh1 force-pushed the deb-repositories-and-remotes branch from 9772856 to 9ce27a7 Compare September 15, 2026 10:35

warisshaikh1 mentioned this pull request Sep 15, 2026

Share one SyncModal between the repository sync actions #363

Open

		@@ -0,0 +1,149 @@
		import { msg, t } from '@lingui/core/macro';

		@@ -0,0 +1,71 @@
		import { PulpAPI } from './pulp';

		export class DebRemoteType {

		@@ -0,0 +1,42 @@
		import { PulpAPI } from './pulp';

		export class DebRepositoryType {

Uh oh!

Conversation

warisshaikh1 commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this adds

What it deliberately leaves out

The one place deb genuinely differs from file

Testing

Two things I'd flag for review

Uh oh!

Redtigercod4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

warisshaikh1 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

warisshaikh1 commented Aug 28, 2026 •

edited

Loading

Redtigercod4 left a comment •

edited

Loading