JasonEtco / create-an-issue

A GitHub Action for creating a new issue from a template file.
MIT License
267 stars 83 forks source link

Support repo.repo and repo.owner as template variables #114

Closed NathanielRN closed 3 years ago

NathanielRN commented 3 years ago

Description

Adding the repo: tools.context.repo, to the templateVariables allows us to use {{ repo.owner }} and {{ repo.repo }} in the issue template mark down.

One example useful test-case with this is for constructing links to the gh-pages static web pages without having to hardcode these parameters or manipulate the GITHUB_REPOSITORY variable:

e.g. https://{{ repo.owner }}.github.io/{{ repo.repo }}/path/to/my/pageindex.html

Fixes #106

dblock commented 3 years ago

I see ...tools.context already merged here, which I presume is not bringing in its nested structure? It would probably be more future-proof to flatten that entire thing, rewriting keys to include their parent values (e.g. repo.owner) once and for all.

Either way care to add some tests and document the change in README?

Finally, what are other keys in there? Any secrets we might be inadvertently exposing/logging?

NathanielRN commented 3 years ago

@dblock

which I presume is not bringing in its nested structure?

Correct, it is not bringing in the nested structure. Specifically, it is not bringing in the get repo() getter which has two important variables repo.owner and repo.repo we are missing.

It would probably be more future-proof to flatten that entire thing

The available variables on the tools.context depends on the current event. For example there is a tools.context.issue and tools.context.pullRequest but they are obviously not available on a schedule: GitHub workflow event. The tools.context.repo is always available so that is why it made sense to pull only it out. There's nothing else that we need to flatten out from tools.context.

So .repo is the only value that is consistently populated but which was missing from the template context. We don't need to flatten anything else.

care to add some tests and document the change in README?

Updating the README is a great idea, I actually spent a lot of time confused because the README implied that all the values from this list were available on the template but as I found out they were not.

For tests, the current tests don't have anything special for all the different template variables so I didn't think it was necessary to add any new ones. This is something that should simply work especially since we count on GitHub not to change the context in a breaking way any time soon.

Finally, what are other keys in there? Any secrets we might be inadvertently exposing/logging?

Discussed above already, you can see the keys in actions-toolkit:

export declare class Context {
    /**
     * Webhook payload object that triggered the workflow
     */
    payload: WebhookPayloadWithRepository;
    /**
     * Name of the event that triggered the workflow
     */
    event: string;
    sha: string;
    ref: string;
    workflow: string;
    action: string;
    actor: string;
    constructor();
    get issue(): {
        issue_number: number;
        owner: string;
        repo: string;
    };
    get pullRequest(): {
        pull_number: number;
        owner: string;
        repo: string;
    };
    get repo(): {
        owner: string;
        repo: string;
    };
}

We already had everything except for the "getter" methods. I guess maybe it was assumed ...tools.context would include those getters? Either way, only repo is always available, so this PR makes it available to anyone to use.

I am already successfully using this code from my fork to use {{ repo.repo }} in a generalized template.

JasonEtco commented 3 years ago

👋 thanks for opening this @NathanielRN, and thanks @dblock for sharing some 💭 s 🙏

I guess maybe it was assumed ...tools.context would include those getters?

Yes, this was my assumption - but that's not actually true. Or (and bear with me, I wrote most of this Action a long time ago), the reasoning for not requiring that context.repo be available is that since we use an issue template, and those exist within a repo, you could just hard-code those in the issue template. I have no problem including context.repo, but I don't think its strictly necessary. I think this should be merged anyway, but I'd love to hear a use-case where hard-coding it in the issue template won't work.

Would you mind adding a test, for specifically context.repo? That way we can't accidentally remove it down the line.

NathanielRN commented 3 years ago

@JasonEtco

Thanks for you input on this! 😄

you could just hard-code those in the issue template

Yes that's very true, but I found in my use case we could avoid code change if we had access to these variables. Using these variables I was able to copy and paste the same template across multiple repos without any change:

etc...

All these repos use the same issue template, so some future work could include pushing these to their own repo where we can pull them in as a submodule. We wouldn't need to modify them at all if we had the context.repo value.

We use this value to construct a path to a page on GitHub pages which needs the organization and repository name separate, something like this:

https://{{ repo.owner }}.github.io/{{ repo.repo }}/path/to/my/pageindex.html

I added this to the test so that it would be obvious to others who try to use it!

Didn't realize the test were so advanced that they created and compared against snapshots 😄 Really cool, I've added a test for this now!

JasonEtco commented 3 years ago

Published in https://github.com/JasonEtco/create-an-issue/releases/tag/v2.6.0! Thanks again for the contribution 🙌