Adds support for different allowlists in different workflows. #119

ellisonmarks · 2024-01-31T11:01:22Z

No description provided.

lib/tacos/path_filter.py

lib/tacos/dependent_slices.py

lib/tacos/path_filter.py

.github/workflows/tacos_apply.yml

bukzor · 2024-01-31T16:32:03Z

Good stuff. Thanks :) Ding me for round two.

bukzor · 2024-02-21T20:06:53Z

.github/workflows/tacos_apply.yml

@@ -61,6 +61,9 @@ jobs:
      - name: List Slices
        id: list-slices
        uses: ./tacos-gha/.github/actions/list-slices
+        with:
+          allowlist: >-


surrounding style:

Suggested change

allowlist: >-

allowlist: |-

bukzor · 2024-02-21T20:09:05Z

.github/actions/list-slices/action.yml

@@ -26,6 +29,6 @@ runs:
      id: list-slices
      shell: ${{inputs.shell}}
      run: |
-        "$TACOS_GHA_HOME/"lib/ci/list-slices <<'EOF'
+        "$TACOS_GHA_HOME/"lib/ci/list-slices ${{inputs.allowlist}} <<'EOF'


If you instead pass this via an environment variable, it avoids the possibility of weird characters making the bash parser confused.

Suggested change

"$TACOS_GHA_HOME/"lib/ci/list-slices ${{inputs.allowlist}} <<'EOF'

env:

allowlist: ${{inputs.allowlist}}

run: |

"$TACOS_GHA_HOME/"lib/ci/list-slices "$allowlist" <<'EOF'

It probably doesn't matter here, but it's quite important elsewhere so it's a good habit.

bukzor · 2024-02-21T20:11:36Z

.github/workflows/tacos_detect_drift.yml

+          slices="$("./tacos-gha/"lib/tacos/slices \
+                      .config/tacos-gha/drift.allowlist | jq -R | jq -sc)"


nit:

Fixed-width indents only, please. As soon as this is no longer a one-liner, I'd continuate each |.

Suggested change

slices="$("./tacos-gha/"lib/tacos/slices \

.config/tacos-gha/drift.allowlist | jq -R | jq -sc)"

slices="$(

"./tacos-gha/"lib/tacos/slices \

.config/tacos-gha/drift.allowlist |

jq -R |

jq -sc

)"

bukzor · 2024-02-21T20:11:59Z

.github/workflows/tacos_plan.yml

@@ -60,6 +60,10 @@ jobs:
      - name: List Slices
        id: list-slices
        uses: ./tacos-gha/.github/actions/list-slices
+        with:
+          allowlist: >-


Suggested change

allowlist: >-

allowlist: |-

bukzor · 2024-02-21T20:16:42Z

lib/tacos/dependent_slices.py

+
+    args = argv[1:]
+    # need to modify argv for fileinput to work
+    del argv[1:]

    fs = FileSystem.from_git()
    modified_paths = lines_to_paths(fileinput.input(encoding="utf-8"))


fileinput is only useful where we want to not care whether the user is using stdin or positional arguments.
Since we can no longer use positional arguments for file inputs, fileinput is unecessary complextiy and this would be expressed more simply as:

Suggested change

modified_paths = lines_to_paths(fileinput.input(encoding="utf-8"))

from sys import stdin

...

modified_paths = lines_to_paths(stdin)

and then there's no need to modify argv, either.

bukzor · 2024-02-21T20:18:47Z

lib/tacos/path_filter.py

-                        lines.append(line)
-        except FileNotFoundError:
-            pass
+        for path in [OSPath(x) for x in configs] + [OSPath(DEFAULT_PATH)]:


Normally I'd expect a system to only use a default config file when nothing more specific has been specified by the user.

bukzor · 2024-02-21T20:19:20Z

lib/tacos/path_filter.py

+    from sys import argv

-    path_filter = PathFilter.from_config()
+    args = argv[1:]
+    # need to modify argv for fileinput to work
+    del argv[1:]
+
+    path_filter = PathFilter.from_config(args)

    for line in fileinput.input(encoding="utf-8"):


bukzor · 2024-02-21T20:21:50Z

lib/tacos/path_filter.py

+                        line = line.strip()
+                        if line and not line.startswith("#"):
+                            lines.append(line)
+                break


This is only making use of the first config file even if the user passed two? I would normally expect the system to concatenate the configs in this case.

If you treat a missing file as equal to an empty file I believe you get the same, correct behavior but with a bit fewer edge cases to consider.

bukzor · 2024-02-21T20:23:20Z

lib/tacos/path_filter.py

        """Get a list of allowed globs from a file

        Hash comments are removed and blank lines are ignored.
        Inline comments are not allowed
-        An empty or missing file is treated as all allowed.
+        If both the provided and default files are missing, allow all.


Suggested change

If both the provided and default files are missing, allow all.

If all configuration files are empty or missing, allow all.

bukzor · 2024-02-21T20:25:45Z

.github/workflows/tacos_apply.yml

@@ -61,6 +61,9 @@ jobs:
      - name: List Slices
        id: list-slices
        uses: ./tacos-gha/.github/actions/list-slices
+        with:
+          allowlist: >-
+            .config/tacos-gha/apply.allowlist .config/tacos-gha/drift.allowlist


Suggested change

.config/tacos-gha/apply.allowlist .config/tacos-gha/drift.allowlist

.config/tacos-gha/apply.allowlist

.config/tacos-gha/drift.allowlist

bukzor requested changes Jan 31, 2024

View reviewed changes

Adds support for different allowlists in different workflows.

04db0dc

ellisonmarks force-pushed the emarks/separate-allowlists branch from 219f50d to 897f20f Compare February 16, 2024 10:18

Incorporates suggestions and handles the drift workflow.

b2eead7

ellisonmarks force-pushed the emarks/separate-allowlists branch from 897f20f to b2eead7 Compare February 16, 2024 10:22

ellisonmarks requested a review from bukzor February 16, 2024 10:23

Makes plan obey the apply and drift lists, and apply obey drift.

19c5ec2

ellisonmarks force-pushed the emarks/separate-allowlists branch from f9c449f to 19c5ec2 Compare February 20, 2024 10:43

Merge branch 'main' into emarks/separate-allowlists

542a900

bukzor reviewed Feb 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for different allowlists in different workflows. #119

Adds support for different allowlists in different workflows. #119

ellisonmarks commented Jan 31, 2024

bukzor commented Jan 31, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

bukzor Feb 21, 2024

-        "$TACOS_GHA_HOME/"lib/ci/list-slices ${{inputs.allowlist}} <<'EOF'
+  env:
+    allowlist: ${{inputs.allowlist}}
+  run: |
+        "$TACOS_GHA_HOME/"lib/ci/list-slices "$allowlist" <<'EOF'

		slices="$("./tacos-gha/"lib/tacos/slices \
		.config/tacos-gha/drift.allowlist \| jq -R \| jq -sc)"

-    modified_paths = lines_to_paths(fileinput.input(encoding="utf-8"))
+    from sys import stdin
+    ...
+    modified_paths = lines_to_paths(stdin)

	If both the provided and default files are missing, allow all.
	If all configuration files are empty or missing, allow all.

	.config/tacos-gha/apply.allowlist .config/tacos-gha/drift.allowlist
	.config/tacos-gha/apply.allowlist
	.config/tacos-gha/drift.allowlist

Adds support for different allowlists in different workflows. #119

Are you sure you want to change the base?

Adds support for different allowlists in different workflows. #119

Conversation

ellisonmarks commented Jan 31, 2024

bukzor commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment