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

fix: random key for each collection job #1135

Merged
merged 8 commits into from
Jul 15, 2024
Merged

Conversation

gnarf
Copy link
Contributor

@gnarf gnarf commented Jun 27, 2024

I think I finally got through most of the troubles I was having with this PR...

This removes the local "mock http server" for automation scheduler, as we aren't using that HTTP API to spawn jobs, just calling directly to github, we have a similar method to start a mock job to the one that starts the github job.

Each collectionJob now generates a unique UUID for it's secret which is passed along to the job runner, and only usable for one collection job


gnarf added 2 commits June 26, 2024 20:36
Squashed commit of the following:

commit 12ae1e0
Author: Mx. Corey Frang <[email protected]>
Date:   Mon Jun 17 14:19:08 2024 -0400

    got a little further, still blocked on tests

commit d54ffad
Author: Mx. Corey Frang <[email protected]>
Date:   Mon Jun 17 11:28:10 2024 -0400

    debuggering

commit 7bfa8f8
Author: Mx. Corey Frang <[email protected]>
Date:   Thu May 30 11:59:02 2024 -0400

    turbo logging trying to track down a bug

commit 842a459
Author: Mx. Corey Frang <[email protected]>
Date:   Wed May 29 12:21:15 2024 -0400

    remove server abstraction layer
@gnarf gnarf requested review from howard-e and stalgiag June 27, 2024 04:02
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

This is great! Thank you!

I tested locally and with the actual GH runner. All works as expected. I left a few small comments inline. Nothing big. Will approve after they are addressed or discussed.

.json({ error: `Could not find job with jobId: ${id}` });

// store the collection job on the request to avoid a second lookup
req.collectionJob = job;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering how the collectionJob was going to get on the req when I looked at AutomationController before this. Thanks for the comment explainer

},
secret: {
type: DataTypes.UUID,
allowNull: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set defaultValue: uuid() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would lead to a single default uuid per time the app started up i think...

The beforeValidate hook was the most recommended and stable feeling suggestion I ran across for dealing with default values with uuid()

Copy link
Contributor

@stalgiag stalgiag Jul 11, 2024

Choose a reason for hiding this comment

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

Oh right that makes sense. Would Sequelize's UUIDV4 be useful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a few variants of the sequelize UUID stuff, the version I landed on was the one that made the "cleanest" sense of what was happening I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks for trying!

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Looks good! Confirmed changes work as expected locally as well and the reorganization of things look good to me. Left some minor inline comments

@gnarf gnarf requested a review from stalgiag July 11, 2024 19:01
@gnarf gnarf merged commit 78fab77 into development Jul 15, 2024
2 checks passed
@gnarf gnarf deleted the x-automation-secret branch July 15, 2024 16:19
howard-e added a commit that referenced this pull request Jul 22, 2024
Includes the following changes:

**Features and Fixes**
* #1125 
* #1135
* #1144
* #1146
* #1150
* #1152
* #1153
* #1160

**Infrastructure changes**
* #969
* #1151
* #1148

---------

Co-authored-by: alflennik <[email protected]>
Co-authored-by: Mx Corey Frang <[email protected]>
Co-authored-by: cypress evelyn masso <[email protected]>
Co-authored-by: Stalgia Grigg <[email protected]>
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.

3 participants