addSecrets() overwrites existing database settings and never marks secrets as added

Open Beginner friendly
  • Is something wrong?
#4,235 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
84/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
r
Domain
backend

Research direction

Start with base/settings/R/addSecrets.R, especially the checks at lines 19–21 and 36–44 described in the issue. Add a regression test for existing database settings and for the secrets.added guard, using a temporary ~/.pecan.xml as suggested. Done means existing values are preserved and the added flag prevents secrets from being applied again.

Written by the indexing model from the issue text.

Description

addSecrets() is meant to copy database sections from ~/.pecan.xml only when the settings don't already have them, but it overwrites them every time.

# ~/.pecan.xml has <database><bety><user>from_pecan_xml</user><host>localhost</host>...</bety></database>
s <- PEcAn.settings::as.Settings(list(database = list(bety = list(
  user = "mine", password = "x", host = "db.example.org", dbname = "bety"))))
out <- PEcAn.settings::addSecrets(s)
out$database$bety$user                 #> "from_pecan_xml"
out$database$bety$host                 #> "localhost"
is.null(out$settings.info$secrets.added)  #> TRUE

(develop at 9fa7807, PEcAn.settings 1.9.1.9000, macOS arm64, R 4.5.2)

The check looks for the section among the top-level names instead of under database:

https://github.com/PecanProject/pecan/blob/9fa7807a4908fd4aa048d171054088b33fa50f15/base/settings/R/addSecrets.R#L36-L44

settings[section] with section = "bety" is never a top-level entry, so the "Already have a section" branch never runs and settings$database$bety is replaced from ~/.pecan.xml. Separately, settings.info$secrets.added is checked at L19-21 but never set, so the "added already" guard never triggers either.

Suggested fix: check section %in% names(settings[[key]]), and set settings$settings.info$secrets.added <- TRUE before returning, plus a test that uses a temporary ~/.pecan.xml (e.g. via withr::local_envvar(HOME = ...)) for both cases.

I didn't find an existing issue. Happy to open a PR if this sounds right.

Dominant language
R
Stars
246
Forks
322
Avg merge
5d 9h
Merged PRs (30d)
21

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from PecanProject/pecan

All issues in PecanProject/pecan

Similar issues

More R issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.