Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add zones input variable to optional sync a subset of zones in the configuration #104

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

maikelpoot
Copy link
Contributor

Description

This PR will add the a zones input to utilize the "zone" parameter from octodns-sync to only sync specific zones from the configuration.

Motivation and Context

We use the octodns-sync action to dry-run the zone changes in a PR.
But in most PR's only 1 or 2 zones are changed to it's a bit wasteful in api-calls and time to dry-run all the zones and not only the changed zones.
The octodns-sync application has support for this but the github action did not yet.

How Has This Been Tested?

This has been tested by creating a fork, and switching our real-world (staging and production) repo's over to the fork.
In combination with the tj-actions/changed-files github action, our PR's only dry-run altered zone yaml files.

Snippet from our GithubAction

on:
  pull_request:
    paths:
      - '/zones/*'

jobs:
  octodns-dryrun:
      ..........     
      - name: Get all changed zone files
        id: changed-zone-files
        uses: tj-actions/changed-files@v45
        with:
          files: |
            zones/**.yaml
      - name: List all changed zone files
        id: create-zones
        if: steps.changed-zone-files.outputs.any_changed == 'true'
        env:
          ALL_CHANGED_FILES: ${{ steps.changed-zone-files.outputs.all_changed_files }}
        run: |
          for file in ${ALL_CHANGED_FILES}; do
            zone=$(basename "$file" .yaml)
            zones="${zones} ${zone}."
          done
          echo ZONES=${zones} >> $GITHUB_OUTPUT
      - uses: maikelpoot/octodns-sync@zones
        id: generate-plan
        with:
          config_path: ./config.yaml
          force: 'Yes'
          zones: "${{ steps.create-zones.outputs.ZONES }}"
        env:
          ** Secrets **
      ..........

Anything else?

Output of real PR (domain and target are redacted):

INFO  Manager __init__: config_file=./config.yaml, (octoDNS 1.3.0)
INFO  Manager _config_executor: max_workers=1
INFO  Manager _config_include_meta: include_meta=False
INFO  Manager _config_auto_arpa: auto_arpa=False
INFO  Manager __init__: global_processors=[]
INFO  Manager __init__: global_post_processors=[]
INFO  Manager __init__: provider=yamlgitops (octodns.provider.yaml 1.3.0)
INFO  Manager __init__: provider=redacted (octodns_redacted 0.0.0)
INFO  Manager __init__: plan_output=html (octodns.provider.plan 1.3.0)
INFO  Manager sync: eligible_zones=['example.com.'], eligible_targets=[], dry_run=True, force=True, plan_output_fh=<stdout>
INFO  Manager sync:   zone=example.com.
INFO  Manager sync:   sources=['yamlgitops']
INFO  Manager sync:   targets=['redacted']
INFO  YamlProvider[yamlgitops] populate:   found 20 records, exists=False
INFO  RedactedProvider[redacted] plan: desired=example.com.
INFO  RedactedProvider[redacted] populate:   found 13 records
INFO  RedactedProvider[redacted] plan:   Creates=7, Updates=1, Deletes=0, Existing Records=13

This sync only took 4 seconds to run, before (with all zones) a sync would take about 2.5 minutes (159 seconds).

Copy link
Owner

@solvaholic solvaholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for working this out and suggesting the change, @maikelpoot! 🙇 Please forgive my delays in following up on your pull request.

I'm a little concerned that we're relying on the runner's /bin/bash to handle space-separated zones. Is it safe to assume any runner a user selects in their workflow will have a /bin/bash that does the needful?

The steps below failed in my macOS workstation's default /bin/zsh:

% _config=fixtures/configs/dynamic-zones.yaml
% ZONES="test1.octodns-sync. test2.octodns-sync."
% _zones=$ZONES
% octodns-sync --config-file=$_config $_zones
[...]
octodns.manager.ManagerException: Requested zone "test1.octodns-sync. test2.octodns-sync." not found in config

However, the same steps succeeded in /bin/bash. I'm OK with merging the input as you wrote it, and re-evaluating if anyone has trouble using it.

Before we merge it, please add the new input to README.md:

### `zones`

Space separated list of zones to sync, leave empty to sync all zones in the config file.

Default: `""` (empty string)

And to docs/CHANGELOG.md:

## [Unreleased]

### Added

- ([#104](https://github.com/solvaholic/octodns-sync/pull/104)) Add input **zones** to allow syncing a subset of configured zones.

(I did try to make those doc changes myself, and did not have permission to update maikelpoot:zones.)

@maikelpoot
Copy link
Contributor Author

maikelpoot commented Nov 4, 2024

Please forgive my delays in following up on your pull request.

No worries, we are all busy :)

Is it safe to assume any runner a user selects in their workflow will have a /bin/bash that does the needful?

It should as your action defines the command to be run with the bash shell.
Also there is a shebang for bash in run.sh so that should make it safe to assume bash syntax is safe to use.

For zsh the line could be altered to ${=_zones} to enforce word splitting, but bash will fail on that syntax (see examples):

please add the new input to README.md:
(I did try to make those doc changes myself, and did not have permission to update maikelpoot:zones.)

Yeah sorry, as i changed our production repo to use "my" fork/branch it seemed best to disable that option.

Added the requested changes to the documentation.

test.sh:

X="A B C"
echo "Normal: ./test2.sh \${X}"
./test2.sh ${X}
echo "Word splitting: ./test2.sh \${=X}"
./test2.sh ${=X}

test2.sh:

% cat test2.sh
#!/bin/zsh
echo 1 $1
echo 2 $2
echo 3 $3
% /bin/zsh test.sh
Normal: ./test2.sh ${X}
1 A B C
2
3
Word splitting: ./test2.sh ${=X}
1 A
2 B
3 C

% /bin/bash test.sh
Normal: ./test2.sh ${X}
1 A
2 B
3 C
Word splitting: ./test2.sh ${=X}
test.sh: line 5: ${=X}: bad substitution

edit: split quote and reply

Copy link
Owner

@solvaholic solvaholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @maikelpoot! 🙇

@solvaholic
Copy link
Owner

Thanks for adding those, and thinking through this:

Is it safe to assume any runner a user selects in their workflow will have a /bin/bash that does the needful?

It should as your action defines the command to be run with the bash shell.

Given that and the #! in run.sh, the one risk I envision is if a user's using a Windows or self-hosted runner that doesn't have a compatible /bin/bash. I think that seems super unlikely, so I don't really worry about it .. am content to have ack'd the risk and thought it through.

@solvaholic solvaholic merged commit 4031e5e into solvaholic:main Nov 4, 2024
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.

2 participants