Skip to content

Conversation

@ccbblin
Copy link
Contributor

@ccbblin ccbblin commented Apr 23, 2025

Motivation

The create-config command of the cogify package should live alongside the other basemaps-config-related commands in the new cli-config package.

Modifications

  1. Relocate the cogify-config command from the cogify package to the cli-config package.
  2. Import the command into the cli package as a config create sub-command.
  3. We've moved some utility functions to the shared package. So, this work also updates various functions to use the new shared equivalents instead, removing the singular copies in the process.

2b82d71c-b820-410f-b05b-4b323f90745f

Verification

check bin/bmc.js config create-config runs correctly

@ccbblin ccbblin force-pushed the feat/move-cogify-create-config-into-cli-config-package branch from 5ea4ca1 to 3f259c6 Compare April 23, 2025 02:35
@ccbblin ccbblin marked this pull request as ready for review April 23, 2025 02:50
@ccbblin ccbblin requested a review from a team as a code owner April 23, 2025 02:50
@ccbblin ccbblin force-pushed the feat/move-cogify-create-config-into-cli-config-package branch from b0780de to df954ea Compare April 23, 2025 21:26
@tawera-manaena
Copy link
Contributor

tawera-manaena commented Apr 24, 2025

packages/cli-config/src/cli/action.create.config.ts

@Wentao-Kuang - Should we adjust the paths '/tmp/cogify/config-*' now that this command lives in the cli-config package?

...
    if (isArgo()) {
      // Path to where the config is located
      await fsa.write(fsa.toUrl('/tmp/cogify/config-path'), urlToString(outputPath));
      // A URL to where the imagery can be viewed
      await fsa.write(fsa.toUrl('/tmp/cogify/config-url'), urlToString(url));
    }
...

Update: We'll leave the paths as they are. No changes.

Wentao-Kuang
Wentao-Kuang previously approved these changes Apr 24, 2025
@tawera-manaena
Copy link
Contributor

@Wentao-Kuang - Which way should we be initialising the logger for each command?

Most of them (e.g. action.bundle.assets.ts) do it this way:

const logger = getLogger(this, args, 'cli-config');

But the action.import.ts command does it this way:

const logger = LogConfig.get();

Is one way better than the other?

@Wentao-Kuang
Copy link
Contributor

Member

const logger = LogConfig.get(); is the default way of getting logger for other packages.
The const logger = getLogger(this, args, 'cli-config'); should be used for cli packages that include the cli versions/packages infor in the logger.

@tawera-manaena
Copy link
Contributor

const logger = LogConfig.get(); is the default way of getting logger for other packages. The const logger = getLogger(this, args, 'cli-config'); should be used for cli packages that include the cli versions/packages infor in the logger.

Good to know. I'll update the action.import.ts command now.

@ccbblin ccbblin added this pull request to the merge queue Apr 24, 2025
Merged via the queue into master with commit 5f72430 Apr 24, 2025
16 checks passed
@ccbblin ccbblin deleted the feat/move-cogify-create-config-into-cli-config-package branch April 24, 2025 03:06
@blacha blacha mentioned this pull request May 11, 2025
github-merge-queue bot pushed a commit that referenced this pull request May 11, 2025
# [v8.0.0](v7.17.0...v8.0.0)

**Please Note this is a work in progress release, please remain on v7
until the command line changes have been finalized**

### Bug Fixes

* **cli:** update landing deploy script with new cli-config package
BM-1260 ([#3431](#3431))
([4d7e86d](4d7e86d)),
closes
[/github.com/linz/basemaps/blob/e527a04fec65c82f8577642493e8fcb475762243/packages/landing/scripts/deploy.mjs#L1](https://github.com//github.com/linz/basemaps/blob/e527a04fec65c82f8577642493e8fcb475762243/packages/landing/scripts/deploy.mjs/issues/L1)
* ensure all linzjs packages are correctly labeled as deps
([#3439](#3439))
([de9df87](de9df87)),
closes [#3438](#3438)
* ensure api key blocks are set in ci/cd
([#3442](#3442))
([28f6403](28f6403))


### Features

* **cli-vector:** Extract cli to load schema json and prepare jobs to
process vector mbtiles. BM-1267
([#3429](#3429))
([db113e2](db113e2))
* **cli:** add cli-config package BM-1260
([#3428](#3428))
([4ca5a47](4ca5a47))
* **cli:** move cogify create-config into cli-config package BM-1261
([#3432](#3432))
([5f72430](5f72430))
* **cli:** rename cogify package to cli-raster BM-1262
([#3433](#3433))
([36d4449](36d4449))
* **cli:** update cli-package commands to cmd-ts BM-1259
([#3427](#3427))
([46cc342](46cc342))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants