Skip to content

Conversation

@MarkAckert
Copy link
Member

@MarkAckert MarkAckert commented May 2, 2025

Proposed changes

Note: Ready for review.

This PR adds a new copyConfigurationAndDeleteKey API to configmgr which can be used to delete keys from a YAML document. e.g., deleting zowe.setup or zowe.setup.dataset is now possible. The copyConfigurationAndDeleteKey API requires that a new configuration name be supplied, so the mutation happens in a new copy of the configuration (matching other update APIs). This is required as part of the Node.JS removal from zwe scripts, and is last step to fully replace the config-converter component we use from zowe-install-packaging-tools.

The existing modification APIs were not used after experimentation, as the modification / overlay is a union style merge operation rather than a replacement. While that could've been enhanced to support deleting keys, it felt better to have a clean line of separation between the 2 behaviors.

Limitations in regex made parsing of keys containing multiple periods less readable than I wanted, so the code has additional comments in the parsing section.

This PR addresses Issue: Part of zowe/zowe-install-packaging#4307

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Change in a documentation
  • Refactor the code
  • Chore, repository cleanup, updates the dependencies.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

  • If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • Relevant update to CHANGELOG.md
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

Added unit tests in tests/js/config1.js.

Further comments

Supports:

  • Deleting composite array and object keys. e.g., both zowe.externalHosts and zowe.externalHosts[0] are supported (and yield different outputs).
  • Deleting composite keys where a key entry contains periods. Syntax: zowe.[_zsf.logging.level]. Works for arrays too: zowe.[_demo.key[0]].[another.key]
zowe:
  _zsf.logging.level: TRACE
  _demo.key:
    - another.key: SOMETHING
  • If the delete key supplied does not exist, this is a no-op, and returns the document as-is. This matches the config-converter behavior.

Notes:

  • If you delete the only property of an object, the render outputs curly braces. e.g.:
zowe:
  a:

Running delete "zowe.a" gives:

zowe: {}

This is legal YAML, but might be confusing if a user comes across it? I don't know how to change the render behavior.

  • Keys with [ in position one will not parse as keys, but instead as a key containing periods, which means they will not match and the resulting output will be a no-op. I think this is an acceptable limitation until we have a use case which needs [ in position one. This might be worked around by: [[key], but is not tested.

@MarkAckert MarkAckert marked this pull request as ready for review May 5, 2025 18:07
@JoeNemo
Copy link
Contributor

JoeNemo commented May 7, 2025

Is this meant to be a persistent change in yaml? Or is this only an in-memory change for the program that has integrated configmgr?

@MarkAckert
Copy link
Member Author

This is an in-memory change only; the caller is responsible for persisting changes to disk. I followed the structure of updateZoweYaml / makeModifiedConfiguration.

These lines in the zowe-install-packaging PR shows the call pattern: https://github.com/zowe/zowe-install-packaging/blob/5726c3792404e72ead99468fe37621348d408e7d/bin/libs/configmgr.ts#L425-L429 , where deleteConfig is a wrapper around the configmgr API. The caller modifies the YAML in-memory, then issues a separate call to write out the changes.

@JoeNemo
Copy link
Contributor

JoeNemo commented May 14, 2025

There really are no "destructive" operations in configmgr to prevent bugs where one consumer/user of the data is not in sync with another. Can we re-conceive this as making a copy of the config under a new name with the following edits applied.

@1000TurquoisePogs
Copy link
Member

You may find https://github.com/zowe/zowe-install-packaging/blob/v3.x/staging/bin/commands/internal/config/get/index.ts zwe internal config set to be an alternative to achieving similar goals. What it does, instead of "deleting", is it reads the existing config, makes an edit, places the edit into a location, and then says to configmgr "now load name_of_config_revision_1" and points our TS code to that as the current config variable.

@MarkAckert
Copy link
Member Author

@JoeNemo This does currently create a copy of the configuration before taking destructive actions; a user always needs to call the API with the next revision name. I'll join the squad call next week to answer other questions that may come up

…rrays indexed by square brackets

Signed-off-by: MarkAckert <[email protected]>
@MarkAckert MarkAckert changed the title add configmgr api deleteFromConfiguration add configmgr api copyConfigurationAndDeleteKey May 22, 2025
MarkAckert and others added 4 commits May 22, 2025 15:45
Signed-off-by: MarkAckert <[email protected]>
Signed-off-by: Martin Zeithaml <[email protected]>
Signed-off-by: Martin Zeithaml <[email protected]>
Signed-off-by: Martin Zeithaml <[email protected]>
@1000TurquoisePogs
Copy link
Member

As review and discussion has been complete and marked approved, I'm going to merge this.

@1000TurquoisePogs 1000TurquoisePogs merged commit c744ce4 into v3.x/staging Jun 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants