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: scope asset publishing scripts inside stage assembly directory #397

Merged
merged 9 commits into from
Feb 12, 2024

Conversation

NathanTCz
Copy link
Contributor

Inspired by #280. Added tests.

Fixes #279

@NathanTCz
Copy link
Contributor Author

@kaizencc Any feedback on this one?

@vincentgna
Copy link

vincentgna commented Feb 9, 2023

I have the same issue (creating multiple GitHubWorkflow for different targets and publish-Assets-FileAssetX-step.sh files getting overwritten. I would think the solution is to include the GitHubWorkflow Stack id to the jobId using the Names.uniqueId(this) function?

-        const jobId = node.uniqueId
+        const jobId = `${Names.uniqueId(this)}-${node.uniqueId}`

@NathanTCz
Copy link
Contributor Author

Fixed conflicts

@diranged
Copy link

diranged commented Sep 4, 2023

@kichik could we get this in? Perhaps add a few maintainers to this repo who are able to review PR’s like this?

@diranged
Copy link

diranged commented Sep 4, 2023

@tj-harris , @mrgrain , @giseburt

diranged added a commit to Nextdoor/cdk-pipelines-github that referenced this pull request Sep 4, 2023
@kichik
Copy link
Contributor

kichik commented Sep 5, 2023

@kichik could we get this in? Perhaps add a few maintainers to this repo who are able to review PR’s like this?

I wish, but I don't work for AWS and have no access to this repo.

@hikeeba
Copy link

hikeeba commented Oct 31, 2023

@kaizencc, can you look at this, or is there someone else we can get on this?

@kaizencc
Copy link
Contributor

kaizencc commented Feb 1, 2024

I can't exactly remember why I stopped working on this besides this project being a side project for me. I seem to recall that it was because we thought that the correct use case here is 1 pipeline, 1 account. But it's been so long ago now that I can't remember exactly what, and it seems like there is at least a handful of people who are doing it this way and running into this problem.

I'll have to look into whether or not changing the file path as this PR attempts makes sense, or if we should update the file name itself.

@diranged
Copy link

diranged commented Feb 1, 2024

@kaizencc,
I ultimately ended up forking this project to https://github.com/Nextdoor/cdk-pipelines-github and simplifying the system ... I removed most of the "publish assets" steps and I just pass around the cdk.out dir between build steps. It's more akin to running yarn cdk deploy ... on a laptop, but with all the benefits of the Github Pipelines.

Honestly at this point, I was considering "launching" this as a public project under a different name because I think it's really valuable. We're using it on 8 different projects internally and generating lovely Github workflow pipelines. I don't mind going back to using the core project, if its well supported.. but as you said, its a "side project" and we can't really rely on a side project that isn't reasonably well supported, ya know?

(we've also added steps and functionality for handling OIDC logins, and using different OIDC credentials for different stages)

@kaizencc
Copy link
Contributor

kaizencc commented Feb 7, 2024

Hi @diranged,

I simply love that you + your team have forked this repo and support additional features. Especially if you use it internally and are likely to support it in the future, I think it would be a great idea to publish your construct. While I would love to say that this project can get enough love, it simply is just me :). I will say though, that this repo gets better support than other repos in cdklabs...

In my view, the future of this repository is as follows:

a) a good Samaritan publishes a better construct with better support and people move on to that. we eventually archive this repo.
b) community members willing to spend time reviewing + contributing features on this repository in tandem with CDK team's stamp-of-approval reviews. We can work out a set of trusted community members similar to what we have in aws-cdk.
c) people raise hell and AWS decides to fully support this project. pretty unlikely, but never say never :).

I am committing to figure something out to fix the issue in this PR, since it seems like a problem for this repo. But I cannot commit to anything more than reviewing PRs in this repository in the future, so that's the current support level of this construct.

I am happy to chat more here or in cdk.dev about solutions for either a) or b)! The faster we land on one of these paths, the better.

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.

@NathanTCz thanks for bearing with me for over a year. Lets send it. The deploy.yml file that this creates seems to make sense, we are just moving around where the files live.

@mergify mergify bot merged commit 9c6934a into cdklabs:main Feb 12, 2024
12 checks passed
@NathanTCz
Copy link
Contributor Author

@kaizencc thanks very much for reviewing this one! We also contemplated forking this package...

@NathanTCz NathanTCz deleted the fix/stage-scoped-asset-scripts branch February 13, 2024 20:57
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.

Script for publishing assets should be stage specific
6 participants