Skip to content

Switch to new labelling scheme for our GitHub runners#35109

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-airflow-runner-label
Oct 23, 2023
Merged

Switch to new labelling scheme for our GitHub runners#35109
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-airflow-runner-label

Conversation

@potiuk
Copy link
Copy Markdown
Member

@potiuk potiuk commented Oct 22, 2023


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Oct 22, 2023

I also improved significantly the output of selective checks unit tests when they see something unexpected:

It was rather cryptic with the regular assertion so I added a bit more custom/coloured output and more accurate and explicit description showing what's expected, what's received and what was the output received (as nicely formatted dict).

Screenshot from 2023-10-22 17-38-14

@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Oct 22, 2023

But I need to make it works:

Requested labels: "\"[\\\"self-hosted\\\", \\\"airflow-runner\\\", \\\"vm-runner\\\", \\\"X64\\\"]\""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't seed the labels attached to the runners; we need to check before merging to avoid blocking the committers' PRs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should passed as an array instead of a string; it will be serialized to JSON by GHA 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't seed the labels attached to the runners; we need to check before merging to avoid blocking the committers' PRs.

Yeah. This one won't get green until that works, so no worries :)

I think this should passed as an array instead of a string; it will be serialized to JSON by GHA 🤔

It's complicated. We can only print strings as outputs and when needed they need to be converted to arrays with "toJSON" method - but its not obvious how so I am playing with it now to get the right combination

🤷 GA woes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Or rather fromJSON 🤦

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can try:

runs-on: ${{ toJSON(steps.selective-checks.outputs.runs-on) }}

in the list of outputs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

your last commit works 🎉

Requested labels: self-hosted, airflow-runner, vm-runner, X64

Copy link
Copy Markdown
Member Author

@potiuk potiuk Oct 22, 2023

Choose a reason for hiding this comment

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

runs-on: ${{ toJSON(steps.selective-checks.outputs.runs-on) }}

Unfortunately not.... Outputs are always Unicode strings - so converting them to JSON just causes subsequent encoding 🤷 ... We need to run fromJSON in all the places where the outputs are used as part of workflow definition ...

I think I got it right now... But indeed our runners do not have the right labels defined and do not pick the jobs up:

Requested labels: self-hosted, airflow-runner, vm-runner, X64
Job defined at: apache/airflow/.github/workflows/ci.yml@refs/pull/35109/merge
Waiting for a runner to pick up this job...

@potiuk potiuk force-pushed the add-airflow-runner-label branch from 80e47fa to bf150c2 Compare October 22, 2023 16:26
@potiuk potiuk changed the title Add airflow runner label Switch to new labelling scheme for our GitHub runners Oct 22, 2023
@potiuk potiuk force-pushed the add-airflow-runner-label branch 5 times, most recently from cbc7091 to f5f396f Compare October 22, 2023 19:06
We have been using a labelling scheme where just "self-hosted" label
was used for our CI jobs to indicate that the job should run on
the "airflow" provided self-hosted runners. Since the ASF infra
now provides self-hosted runners managed by them when we want to
make use of it, we should change the scheme used by us to allow
more fine-grained distinction between the runners.

This will also be helpful when we switch to new runners based on k8s,
where we want to run them side-by-side with the old VM-based runners, it
will allow us to selectively choose which self-hosted runners we want
to use (and possibly dynamically swap between those).
@potiuk potiuk force-pushed the add-airflow-runner-label branch from f5f396f to 8254ad1 Compare October 22, 2023 21:59
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented Oct 23, 2023

There is a small risk that this one will fail in build-image workflow after I merge it in main so keep fingers crossed

@potiuk potiuk merged commit 8c1b105 into apache:main Oct 23, 2023
@potiuk potiuk deleted the add-airflow-runner-label branch October 23, 2023 08:01
potiuk added a commit that referenced this pull request Oct 29, 2023
…35109)

We have been using a labelling scheme where just "self-hosted" label
was used for our CI jobs to indicate that the job should run on
the "airflow" provided self-hosted runners. Since the ASF infra
now provides self-hosted runners managed by them when we want to
make use of it, we should change the scheme used by us to allow
more fine-grained distinction between the runners.

This will also be helpful when we switch to new runners based on k8s,
where we want to run them side-by-side with the old VM-based runners, it
will allow us to selectively choose which self-hosted runners we want
to use (and possibly dynamically swap between those).

(cherry picked from commit 8c1b105)
@potiuk potiuk added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 29, 2023
@potiuk potiuk added this to the Airflow 2.7.3 milestone Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants