-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add support for generating a d2l.dev app #90
Add support for generating a d2l.dev app #90
Conversation
fc55065
to
6d7b4d9
Compare
aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
aws-session-token: ${{ secrets.AWS_SESSION_TOKEN }} | ||
role-to-assume: "arn:aws:iam::022062736489:role/r+<%= githubOrg %>+<%= repoName %>+repo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where best to put this -- maybe in the generated readme or the script output? -- but there should probably be some indication that they'll need to configure this in repo-settings
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on where it was better to indicate this, but I just put it in the script output for now. Let me know if you'd like me to add something to the README as well.
<script src="./index.js" type="module"></script> | ||
<title><%= hyphenatedName %></title> | ||
</head> | ||
<body class="d2l-typography"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worthwhile plugging the router in here and showing how to properly set up a few "pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if I do that in a follow-up PR? I'm not sure how much time I'll have to work on this right now, so I don't want to get too fancy right off the bat. I also didn't include linting, tests, or PR previews for this reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep totally.
role-duration-seconds: 3600 | ||
aws-region: ca-central-1 | ||
|
||
- name: Publish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the publish step, the "d2l.dev" project is very similar to what I'd imagine a generic "application" to be that doesn't publish to d2l.dev
... like the Daylight Site for example. Just wondering if it's worth it to make this so "d2l.dev" specific, or if making it more generic would make it useful for a wider audience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could ask the user if it's for a d2l.dev site and change things dynamically? What would you envision for the generic site? Just leave the d2l.dev specific things blank (role-to-assume
, aws-region
, bucket-path
)? Or remove the steps altogether?
I'm not sure what is the norm for a generic site, would we still assume that they use BrightspaceUI/actions/publish-to-s3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably leave the publish step out altogether since it's difficult to guess what they'd want. Or we could ask something like "Would you like to publish your application to S3?" (yes/no) and then ask for the bucket?
At some point though there's diminishing returns on the complexity it all adds to the generator since there's already good documentation for S3 publishing and no matter what they'll need to set things up in repo-settings
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, based on your suggestion, I changed the generator (and the template files) to be more generic. It should now change the publish.yml and the readme a little based on what the publishing target is.
127f9bd
to
d1c41c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for adding this!
(make sure you square merge with |
🎉 This PR is included in version 3.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.