Default to dry-run operations

[rappizs]

Fixes #36 although I'm not sure if this documentation is enough or not

Dev notes:
- added missing fields to the update logic
- made all fields optional so empty fields won't be added to the user's config file

• refactored dry run logic, now dry run is the default mode
• updated docs

[codecov]

Codecov Report

Merging #148 (d15b384) into main (e30d191) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #148   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          1       1           
  Lines         67      67           
=====================================
  Misses        67      67           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[justaugustus]

@rappizs -- Thanks for submitting this!

I think this PR may be conflating two features/bug fixes:

• making sure the config file remains consistent: Subsequent runs of tool do not have correct config file fields set #43
• documenting/enabling dry-run operations: Document dry-run operations #36

I'd suggest splitting the "pass by value" / "pass by reference" out into a separate PR, as that looks to be mergeable by itself.

[justaugustus]

(First, flagging that this is related to #36.)

I wrestle with this every time I deal with dry-run operations.
It's been a bit since I've touched this code, but I believe the reason that dry-run is not included as an option in this config is because of the default values.

If you include dry-run as a boolean field and do not set it, then it is initialized as the boolean type's zero value (which is false).
In other words, this means dry-runs must be explicitly requested (and the behavior you actually want is the other way around).

In tools like peribolos, we instead prefer a --confirm flag/option, which must be explicitly set to true for a production run: https://github.com/uwu-tools/peribolos/blob/c84a844017865578458a626222eacbaefa01817c/README.md?plain=1#L226-L228

[rappizs]

I totally agree with the --confirm approach but for this refactoring we should create a separate issue in my opinion.

[justaugustus]

we should create a separate issue in my opinion

I'd suggest splitting the "pass by value" / "pass by reference" out into a separate PR, as that looks to be mergeable by itself.

@rappizs -- I agree :) ref: #148 (review)

That said, we should either:

• introduce a non-destructive option
• not introduce the option

Defaulting dry-run = false is destructive, so it must not introduced.

[rappizs]

Agreed, but it's already introduced, dry-run is a supported config option ATM it's just not documented and it always gets deleted from the config after every run because it's missing from this struct, which we use to update the config file. But it's handled and used by the business logic with or without this PR:

gh-jira-issue-sync/cmd/root.go

Line 74 in 4b64ef0

if !cfg.IsDryRun() {

Should I add the dry-run refactoring logic to this PR as well?

[justaugustus]

Yeah, I think the reason I explicitly left it out of the config struct was you don't want to run into a scenario where you're doing testing and the config file is set to do a production run. The way it exists today forces the user to explicitly set the desired behavior from the command line.

I'm fine with a refactor to include it in the struct here, so long as the default behavior is confirm: false.

[rappizs]

The way it exists today forces the user to explicitly set the desired behavior

Unfortunately not, if the user doesn't specifically set the dry-run property to true in the config file, then it will be a production run. Since it's not a mandatory config field, it will be set to false by default as you mentioned above. But this behavior is completely independent from this PR, this is how it works currently, production run is the default, dry-run is an extra option.

[rappizs]

The option itself is defined here, and used by the business logic everywhere:

gh-jira-issue-sync/internal/options/options.go

Line 51 in 4b64ef0

ConfigKeyDryRun = "dry-run"

Leaving out this field from the struct that we are talking about only results in that this field will be deleted from the config file after every run, which is very confusing.

[rappizs]

@justaugustus refactored the dry run logic, changed the config value to "confirm", now dry run is the default