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

feat: enable all job settings #914

Closed
wants to merge 2 commits into from
Closed

Conversation

clairemr
Copy link

@clairemr clairemr commented Mar 11, 2024

Fixes #915 . Open to feedback if there's a better way to do this, just tried to keep it simple.

I've added @schemastore/github-workflow to get the typing for concurrency, but could also use that to get the type for Job instead of having to maintain that in this project

@clairemr clairemr changed the title update job settings types Feat: enable all job settings Mar 11, 2024
@clairemr clairemr changed the title Feat: enable all job settings feat: enable all job settings Mar 11, 2024
@clairemr clairemr mentioned this pull request Mar 11, 2024
@clairemr
Copy link
Author

fyi this pr would also solve the immediate problem for needing concurrency, but doesn't future proof against the need for customising other job settings in future

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! This PR brings up two threads that are worth thinking about:

  • introducing a third-party library to generate types. maybe this is me being an old grandpa shaking my fist but i don't like giving up control of what our types look like. i would rather us be explicit about what our types are and not allow them to be changed without our own approval. but, previously typing this as unknown was bad too.

  • future-proofing job settings. I am also not a fan of future-proofing as a blanket "good" thing to do. I like to control our API ergonomics and expose only properties with valid use cases for our specific library. I.e. JobSettings in cdk-pipelines-github is not meant to be a mirror of all properties of Job, they are meant to include a subset of properties that our users may wish to configure (without cluttering the type with other useless things). In this case, I currently only see if and concurrency as valid use cases to specify in JobSettings, and I remain convinced that we don't want to expose any more props in the name of future-proofing.

Happy to start a dialogue about both of these threads!

@clairemr
Copy link
Author

clairemr commented Mar 20, 2024

interesting points @kaizencc! thanks for the feedback. I see the logic, but I'd argue that while maintaining control of types is really important for your own constructs, it's an extra overhead to maintain types for someone else's repo. if you have the resources to keep them updated, happy to remove the extra package
re future proofing, that's a pretty reasonable position. the other open pr to expose concurrency matches a lot more closely with your feedback, so if that approach makes more sense I don't mind, just interested in the outcome!

@kaizencc
Copy link
Contributor

Sounds good @clairemr. I will go ahead and merge #905 today to get this in and released. As for the types package, thank you for bringing that to my attention. At least for now, I see more potential problems with introducing that dependency than maintaining our own types. But you are right, that requires some level of maintenance on this project. This project has grown some legs in the past year or so, which is awesome. I am still willing to maintain this repo and at the very least check in 1-2 times a week to make sure that things don't get too stagnant. I'll also look to see if we can utilize community help on this project too. Closing the PR for now, but happy to keep conversing in a github issue or any future PR!

@kaizencc kaizencc closed this Mar 22, 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.

Can't set concurrency
4 participants