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: add templating support #201

Closed
wants to merge 2 commits into from
Closed

feat: add templating support #201

wants to merge 2 commits into from

Conversation

erezrokah
Copy link
Contributor

Hi 👋

It's me again, from this PR #200

Opening as draft to get some feedback.

@@ -123,7 +123,7 @@ class Git {
async createPrBranch() {
const prefix = BRANCH_PREFIX.replace('SOURCE_REPO_NAME', GITHUB_REPOSITORY.split('/')[1])

let newBranch = path.join(prefix, this.repo.branch).replace(/\\/g, '/')
let newBranch = path.join(prefix, this.repo.branch).replace(/\\/g, '/').replace(/\/\./g, '/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix from #200

return true
}

await fs.copy(src, dest, exclude !== undefined && { filter: filterFunc })
await fs.copy(src, dest, { filter: filterFunc })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check for exclude !== undefined inside filterFunc

@BetaHuhn
Copy link
Owner

BetaHuhn commented May 5, 2022

Hey @erezrokah,

first of all, thanks for taking the time to contribute to this project!

Next time, could you explain what your changes are trying to achieve in the PR body? This makes it easier for me as a maintainer to get an overview without me having to go through your changes one by one and having to piece everything together myself.

I think templating support could be useful and I would like to include it in this action, but I am not sure if your current syntax is the right way to go about it. Specifically, I don't like the way you need to specify files to use as data for the templates: <template-name>.<repo>.values.yml.

I created a discussion about this topic where we can discuss how we implement this feature before moving to the actual code. I want to hear about more use cases before finalizing a syntax.

Happy to revisit your idea if you explain your reasoning a bit more. Feel free to comment your ideas in the discussion!

Thanks :)

@erezrokah
Copy link
Contributor Author

Thank you for commenting @BetaHuhn and I appreciate the time you spent going through the PR. I'll definitely add more details next time 🤦 Happy to close the PR and continue this in the discussion 👍

@erezrokah erezrokah closed this May 5, 2022
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.

2 participants