flowey: use typestate pattern for gh variables #643

justus-camp-microsoft · 2025-01-09T23:59:09Z

Instead of hardcoding variable names that may or not be present, we can use the typestate pattern to allow only valid variables at compile time. Motivated by #475, which uses the github.event.pull_request.head.ref in its implementation.

daprilik · 2025-01-13T23:10:09Z

flowey/flowey_lib_common/src/git_checkout.rs

+        let parent_path = ctx
+            .get_gh_context_var()
+            .global(GhContextVar::GITHUB__WORKSPACE);
+        let test_pull_request_event = ctx.get_gh_context_var().event().pull_request();


presumably this will be removed before merge, yeah?

yeah just left in to show it working properly, will remove in a commit that also addresses whatever comments are left

daprilik · 2025-01-13T23:12:25Z

flowey/flowey_core/src/node.rs

@@ -685,10 +706,10 @@ pub trait NodeCtxBackend {
        uses: &str,
        with: BTreeMap<String, ClaimedGhParam>,
        condvar: Option<String>,
-        outputs: BTreeMap<String, Vec<(String, bool)>>,
+        outputs: BTreeMap<String, Vec<(String, bool, bool)>>,


if we do indeed need this extra bool added to these tuples... I think we should use this opportunity to introduce a proper set of struct types to use instead of these tuples, with field names that make the intent of each field clear

daprilik · 2025-01-15T18:28:52Z

flowey/flowey_core/src/node.rs

@@ -838,7 +838,101 @@ pub struct NodeCtx<'a> {
    backend: Rc<RefCell<&'a mut dyn NodeCtxBackend>>,
 }

-impl NodeCtx<'_> {
+#[derive(Serialize, Deserialize)]


all these new types you're adding here should be organized under a new module, to avoid polluting this already messy namespace.

moved into a new module but that also required making some fields public that weren't previously. let me know if there was a better way to go about this

daprilik · 2025-01-15T18:29:44Z

flowey/flowey_core/src/node.rs

+}
+
+impl<'a> GhContextVarReader<'a, ghvarstate::Root> {
+    pub fn global(&self, gh_var: GhContextVar) -> ReadVar<String> {


please make a helper method to avoid duplicating the mostly-identical body between this and other methods

added a helper for the duplicate functionality

daprilik · 2025-01-16T17:36:20Z

flowey/flowey_cli/src/pipeline_resolver/github_yaml/mod.rs

@@ -836,20 +836,28 @@ fn resolve_flow_as_github_yaml_steps(
                    output_steps.push(map.into());
                }

-                for (rust_var, gh_var, is_secret) in rust_to_gh {
+                for gh_var_state in rust_to_gh {
                    let mut cmd = String::new();

                    // flowey considers all GitHub vars to be typed as raw strings


stale comment - not actually true with the introduction of is_object

daprilik · 2025-01-16T17:38:51Z

flowey/flowey_core/src/lib.rs

@@ -7,6 +7,7 @@
 //! this crate!** The crate you should be using is called `flowey`, which only
 //! exports user-facing types / traits.

+pub mod github_context;


this mod should be a mod under mod node, not a new top-level mod.

this should also help with some of the visibility issues you might've run into

good call, this fixed pretty much all the visibility stuff I was having issues with

daprilik · 2025-01-16T17:40:33Z

flowey/flowey_core/src/github_context.rs

+    pub head: Head,
+}
+
+pub enum Root {}


you still want to stuff these ZSTs into their own mod state inline module, just to avoid polluting this namespace with generic-sounding things like Root and Event

daprilik · 2025-01-16T17:40:57Z

flowey/flowey_core/src/github_context.rs

+
+//! Core types and traits used to read GitHub context variables.
+
+use crate::node::{user_facing::GhContextVar, ClaimVar, NodeCtx, ReadVar, StepCtx};


each import on its own line please.

the guide has some suggestions on how to configure rust-analyzer to do this for you

daprilik · 2025-01-16T17:43:43Z

flowey/flowey_core/src/node.rs

@@ -193,8 +194,8 @@ pub enum VarClaimed {}
 /// is possible to infer what order steps must be run in.
 #[derive(Debug, Serialize, Deserialize)]
 pub struct WriteVar<T: Serialize + DeserializeOwned, C = VarNotClaimed> {
-    backing_var: String,
-    is_secret: bool,
+    pub backing_var: String,


yeah, this pub def won't fly 😅

ditto with any other pub you've added to user_facing types

daprilik · 2025-01-16T17:46:25Z

flowey/flowey_core/src/node.rs

@@ -1280,7 +1266,7 @@ impl NodeCtx<'_> {

    #[track_caller]
    #[must_use]
-    fn new_maybe_secret_var<T>(
+    pub fn new_maybe_secret_var<T>(


this is an internal helper - def don't want this public

daprilik · 2025-01-16T17:49:03Z

flowey/flowey_core/src/github_context.rs

+}
+
+impl<'a> GhContextVarReader<'a, Root> {
+    pub fn global(&self, gh_var: GhContextVar) -> ReadVar<String> {


now that you have this read_var helper, I really think that it's not that hard to swap the current constants with standalone helpers - especially since you've already needed to touch those files as part of this global method refactoring.

the only thing you'll need to tweak is the user-defined secret var handling, but even that should be pretty straightforward, if you simply swap the current type that flow uses (i.e: the GhContextVar type we want to go away), with a far more limited newtype around a String called GhUserSecretVar, which is what gets created at the Pipeline level, and plumbed down to a new pub fn secret() method that hangs off this GhContextVarReader.

daprilik · 2025-01-16T19:18:07Z

flowey/flowey_core/src/node/mod.rs

we're not the most consistent with using mod.rs vs. name.rs + name/submod.rs... but I think in this case, we should leave this file as node.rs

daprilik · 2025-01-16T19:21:08Z

flowey/flowey_core/src/node/github_context.rs

+impl<S> GhContextVarReader<'_, S> {
+    fn read_var<T: Serialize + DeserializeOwned>(
+        &self,
+        var_name: String,


this helper can be made marginally more ergonomic by using impl AsRef<str> here. that way - you don't need to invoke .to_string() or .into() when passing &'static str args to it

daprilik · 2025-01-17T18:12:08Z

flowey/flowey_lib_hvlite/src/_jobs/mod.rs

@@ -22,7 +22,6 @@ pub mod build_and_run_doc_tests;
 pub mod build_and_run_nextest_unit_tests;
 pub mod build_and_run_nextest_vmm_tests;
 pub mod cfg_common;
-pub mod cfg_gh_azure_login;


On one hand, I'd prefer if we avoided dropping github-secrets functionality on the ground here, since it seems highly probably that there will come a point in the not-so-far-away future where we'll want to run some stuff in CI that relies on private auth.

On the other hand, yes, we don't currently use this secrets stuff on github, so deleting it in the face of this refactors doesn't "break" anything, and lets us avoid the work of updating the current secret-handling APIs to work with the new github var infrastructure.

I'll let @benhillis chime in on his opinion, as I'm not very cognizant of the time tradeoffs when working on this sort sort of foundational infrastructure, and I don't have a full picture of your other priorities.

My 2c: I already sketched out in another comment how you can port the current gh secret infra to this new foundation, and can discuss it more offline if need be. It don't think it'd be that hard, and it'll make life easier for whoever is next up-to-bat in this space.

Talked offline but I pushed this in a half-broken state (not ready for review) to work on a different machine. I'll reintroduce this file and the API I removed for custom secrets in a commit today.

daprilik · 2025-01-17T18:23:46Z

flowey/flowey_core/src/node.rs

        #[derive(Clone, Debug)]
        pub enum GhParam<C = VarNotClaimed> {
            Static(String),
-            GhVar(GhContextVar),


losing this means we are slightly more inefficient when invoking certain Github Actions from flowey, as we might end up round-tripping a github native variable through a flowey Var for "no reason".

but honestly, that doesn't seem like a big deal, given the perf hit isn't particularly noticible, and could be optimized later if need be.

so this is fine

daprilik · 2025-01-23T22:49:05Z

flowey/flowey_core/src/pipeline.rs

@@ -355,6 +356,9 @@ pub struct Pipeline {
    gh_bootstrap_template: String,
 }

+#[derive(Serialize, Deserialize, Clone)]
+pub struct GhUserSecretVar(pub String);


lets move this to node.rs.

also, we should try to avoid making its guts totally pub. Maybe pub(crate) if need be. but in general, I don't think we want users to really poke around the internal repr of this type after creating it via a Pipeline level API

also, this needs a doc comment

daprilik · 2025-01-23T22:50:24Z

flowey/flowey_core/src/node.rs

@@ -54,6 +57,8 @@ pub mod user_facing {
    pub use crate::flowey_request;
    pub use crate::new_flow_node;
    pub use crate::new_simple_flow_node;
+    pub use crate::node::github_context::state;
+    pub use crate::node::github_context::GhVarState;


this shouldn't be user-facing?

daprilik · 2025-01-23T22:51:48Z

flowey/flowey_core/src/node/github_context.rs

+#[derive(Serialize, Deserialize)]
+pub struct Head {
+    #[serde(rename = "ref")]
+    pub head_ref: String,
+}
+
+#[derive(Serialize, Deserialize)]
+pub struct GhContextVarReaderEventPullRequest {
+    pub head: Head,
+}


these types, which directly correspond to the JSON spec defined by GitHub Actions, should be in their own mod spec, with some doc-comments pointing folks at the docs that define the schema

daprilik · 2025-01-23T22:53:19Z

flowey/flowey_core/src/node/github_context.rs

+use serde::Deserialize;
+use serde::Serialize;
+use std::collections::BTreeMap;
+


I suspect you'll want to have a new pub mod user_facing here which will let you slice-up what public / private parts of this new API.

and then you can pub use github_context::user_facing::* under the node-level user_facing module.

justus-camp-microsoft added 2 commits January 9, 2025 15:50

try and get something working

f4f97df

fmt

82db3d9

justus-camp-microsoft requested review from a team as code owners January 9, 2025 23:59

justus-camp-microsoft added 16 commits January 10, 2025 10:20

test printing pr event

ff116bb

plumb through is_object

46a3016

Merge branch 'main' into ghcontext_typestate

65fed09

try toJSON instead

dcfa810

try echoing instead?

478329b

try another strategy

cbdd984

add parentheses

db8a9c9

let's try jq

c7f63cf

better typestate impl (still needs do_read de-dupe)

e360854

plumb is_object through on write path

ebec5e1

add from_json

d55fc8a

clippy

f778042

see what happens when the event doesn't exist

b71c81b

remove unused events

bfa2abd

remove from_json and add back test event

ff200aa

move event to the right place

1d8e3ff

justus-camp-microsoft changed the title ~~WIP: flowey: use typestate pattern for gh variables~~ flowey: use typestate pattern for gh variables Jan 13, 2025

daprilik reviewed Jan 13, 2025

View reviewed changes

justus-camp-microsoft added 2 commits January 13, 2025 16:33

try another way of passing is_object through

06bade7

add comment about not adding further behavior to GhContextVar

667fbb5

daprilik reviewed Jan 15, 2025

View reviewed changes

justus-camp-microsoft added 3 commits January 15, 2025 10:59

Merge branch 'main' into ghcontext_typestate

86f2354

move to separate module

3aa18b6

shared helper

d88b355

clippy

a5b3562

daprilik reviewed Jan 16, 2025

View reviewed changes

restructure modules, remove test event

1cd416e

daprilik reviewed Jan 16, 2025

View reviewed changes

justus-camp-microsoft added 3 commits January 16, 2025 12:04

move global vars to typestate without removing GhContextVar

c20f33a

remove ghcontextvar

1395a24

submodule structure

7abe491

daprilik reviewed Jan 17, 2025

View reviewed changes

justus-camp-microsoft added 2 commits January 17, 2025 11:21

re-introduce custom secrets flow

3f45028

Merge branch 'main' into ghcontext_typestate

ac30808

daprilik reviewed Jan 23, 2025

View reviewed changes

Justus Camp added 3 commits January 24, 2025 22:03

comments

9f7afd1

docs

440251b

doc tests

a293393

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flowey: use typestate pattern for gh variables #643

flowey: use typestate pattern for gh variables #643

justus-camp-microsoft commented Jan 9, 2025 •

edited

Loading

daprilik Jan 13, 2025

justus-camp-microsoft Jan 13, 2025

daprilik Jan 13, 2025

daprilik Jan 15, 2025 •

edited

Loading

justus-camp-microsoft Jan 15, 2025

daprilik Jan 15, 2025

justus-camp-microsoft Jan 15, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

justus-camp-microsoft Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 16, 2025

daprilik Jan 17, 2025

justus-camp-microsoft Jan 17, 2025

daprilik Jan 17, 2025

daprilik Jan 23, 2025

daprilik Jan 23, 2025

daprilik Jan 23, 2025

daprilik Jan 23, 2025

daprilik Jan 23, 2025


		//! Core types and traits used to read GitHub context variables.

		use crate::node::{user_facing::GhContextVar, ClaimVar, NodeCtx, ReadVar, StepCtx};

flowey: use typestate pattern for gh variables #643

Are you sure you want to change the base?

flowey: use typestate pattern for gh variables #643

Conversation

justus-camp-microsoft commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daprilik Jan 15, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justus-camp-microsoft commented Jan 9, 2025 •

edited

Loading

daprilik Jan 15, 2025 •

edited

Loading