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

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

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

NathanielRN
Copy link
Contributor

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
Copy link
Contributor

dblock commented Sep 27, 2021

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
Copy link
Contributor Author

@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
Copy link
Owner

👋 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
Copy link
Contributor Author

@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!

Copy link
Owner

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

Thanks @NathanielRN - test looks great! I think your use-case makes sense, if only for the portability/easy re-use of templates. Thanks for your contribution 😊 I'll merge this in, but I'm going to wait a bit to release a new version to group it with a couple other minor updates. Shouldn't be long!

@JasonEtco
Copy link
Owner

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

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.

Cannot get repository repo (the name) or repository owner (the org) from template mustache vars
3 participants