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: action workflow to create test URL permits #214

Merged
merged 45 commits into from
Apr 2, 2024

Conversation

gentlementlegen
Copy link
Member

Resolves #195

@gentlementlegen gentlementlegen requested a review from 0x4007 March 29, 2024 07:31
@gentlementlegen
Copy link
Member Author

gentlementlegen commented Mar 29, 2024

A workflow has been added to generate permits that have a value of 0 to any desired wallet. Inputs are:
image

Usable claim payloads are in the build logs:
image

These payloads can be used against the deployed branch, are claimable, usable once and new ones can be generated infinitely, which I think should help for testing. Latest run: https://github.com/gentlementlegen/pay.ubq.fi/actions/runs/8478434201/job/23230754838#step:4:64

@gentlementlegen gentlementlegen marked this pull request as ready for review March 29, 2024 07:34
@gentlementlegen gentlementlegen requested a review from rndquu as a code owner March 29, 2024 07:34
@0x4007
Copy link
Member

0x4007 commented Mar 29, 2024

This approach is decent but the specific problem I was aiming to solve was that I want to be able to test from my phone by clicking the deploy links.

The usability of your implementation is not viable for this use case.

I was thinking we can hard code the deploy action to append the query param for testing purposes.

The app without the passed in permit is pretty useless for testing.

@gentlementlegen
Copy link
Member Author

Of course makes sense. It would be possible to generate the link with the permit and claim in the URL, problem is that it would be only usable once so if it is claimed afterwards you would get a "already claimed" error message. So maybe let's keep this Action for subsequent tests and I'll add it in the build steps so the generated URL also has the claim?

@gentlementlegen
Copy link
Member Author

@pavlovcik I realize that I need a wallet address to generate the permit. Should I default it to yours?

@gentlementlegen
Copy link
Member Author

/query @pavlovcik

Copy link

ubiquibot bot commented Mar 31, 2024

Property Value
Wallet 0x4007CE2083c7F3E18097aeB3A39bb8eC149a341d

@0x4007
Copy link
Member

0x4007 commented Mar 31, 2024

@pavlovcik I realize that I need a wallet address to generate the permit. Should I default it to yours?

0x0 might be the most professional but there's also the admin wallet ubq.eth 0xefC0e701A824943b469a694aC564Aa1efF7Ab7dd

@gentlementlegen
Copy link
Member Author

gentlementlegen commented Mar 31, 2024

So I defaulted it to ubq.eth address to have the UI also behave properly. I manually ran it here
https://github.com/gentlementlegen/pay.ubq.fi/actions/runs/8498117002/job/23277489835#step:6:67
The job fails because there is no related issue to post to. I added a step where a comment is added with the full URL to the current issue.

Edit: it seems the URL for the body is wrongly encoded or contains spurious characters at the end, will investigate.

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member Author

@pavlovcik an example of the comment can be found here: gentlementlegen#11 (comment)

Let me know if the formatting is satisfactory. No new comment is created anymore, and you should be able to open the page simply by clicking on the hash which contains the deployed URL with the permit payload.

@gentlementlegen gentlementlegen requested a review from 0x4007 April 1, 2024 11:51
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

@gentlementlegen Good job

@0x4007
Copy link
Member

0x4007 commented Apr 1, 2024

@pavlovcik an example of the comment can be found here: gentlementlegen#11 (comment)

Let me know if the formatting is satisfactory. No new comment is created anymore, and you should be able to open the page simply by clicking on the hash which contains the deployed URL with the permit payload.

Do you have an example of it being appended? I see it made an edit but isn't it more useful to show all of them? Why don't you just take the logic from this? #214 (comment) source code here.

@gentlementlegen
Copy link
Member Author

The current scenario is that I just update to the latest one because I didn't think it would be relevant to have all the URLs, we are only interested in the final version no? It also avoids doubloons if the workflow is somehow ran twice (two different triggers for example). If that's a bother I can change the logic to have all the deployments.

@0x4007
Copy link
Member

0x4007 commented Apr 1, 2024

The current scenario is that I just update to the latest one because I didn't think it would be relevant to have all the URLs, we are only interested in the final version no? It also avoids doubloons if the workflow is somehow ran twice (two different triggers for example). If that's a bother I can change the logic to have all the deployments.

We have it implemented this way to ease review and debugging. If a bug is introduced with the latest commit, its trivial to check the previous one with our current approach. This was done deliberately to ease reviews and debugging.

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member Author

gentlementlegen commented Apr 1, 2024

@pavlovcik Here is a fix with a sample preview of three different builds in the same comment:
gentlementlegen#13 (comment)
(will address the empty string as well just now)

@gentlementlegen gentlementlegen requested a review from 0x4007 April 1, 2024 16:33
Comment on lines +56 to +111
- uses: actions/github-script@v7
with:
script: |
const { owner, repo } = context.repo;
const sha = "${{ github.event.workflow_run.head_sha }}";

const response = await github.rest.search.issuesAndPullRequests({
q: `repo:${owner}/${repo} is:pr sha:${sha}`,
per_page: 1,
});
const items = response.data.items;
if (items.length < 1) {
console.error('No related PRs found, skipping.');
return;
}
const issue_number = items[0].number;
console.info('Pull request number is', issue_number);

if (!issue_number) {
console.log('Action not triggered from an issue, skipping.');
return;
}

// Fetch existing comments on the issue
const comments = await github.rest.issues.listComments({
owner,
repo,
issue_number,
});

// Find the comment to update or create a new one if not found
let existingComment = comments.data.find(comment => comment.user.login === 'github-actions[bot]');
let body = '| Preview Deployment |\n| ------------------ |\n';

// If the comment exists, update its body
if (existingComment) {
// Check if the SHA already exists in the comment body to avoid duplicates
if (!existingComment.body.includes(sha)) {
body = existingComment.body + `| [${sha}](${{ env.CLAIMABLE_URL }}) |\n`;
await github.rest.issues.updateComment({
owner,
repo,
comment_id: existingComment.id,
body
});
}
} else {
// Create a new comment if no existing comment is found
body += `| [${sha}](${{ env.CLAIMABLE_URL }}) |\n`;
await github.rest.issues.createComment({
owner,
repo,
issue_number,
body
});
}
Copy link
Member

@0x4007 0x4007 Apr 2, 2024

Choose a reason for hiding this comment

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

I generally would recommend importing a standalone typescript module via tsx as we do in our Cloudflare Deploy Action repository. We can handle this in a separate pull.

The reason being that its very easy to make mistakes in this setup, compared to a full standalone TypeScript file with text editor static analysis capabilities (lint, syntax highlighting etc.)

env:
BENEFICIARY_ADDRESS: "0xefC0e701A824943b469a694aC564Aa1efF7Ab7dd"
PAYMENT_TOKEN_ADDRESS: "0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d"
AMOUNT_IN_ETH: 0
Copy link
Member

@0x4007 0x4007 Apr 2, 2024

Choose a reason for hiding this comment

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

I just realized we could make a faucet service if we transfer like 0.0005 WXDAI (instead of permit generation.) This should allow for about 3 transactions.

This could be an interesting plugin for new contributors!

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

I think we can address my comments in separate pulls. Can I see a QA test result?

@gentlementlegen
Copy link
Member Author

@pavlovcik fixed the generate-permit.yml comments on the variable values and names. We can have the script in a file + faucet in another task.

QA results can be seen at gentlementlegen#13 (comment)

@0x4007 0x4007 merged commit 941d461 into ubiquity:development Apr 2, 2024
3 checks passed
Copy link
Contributor

github-actions bot commented Apr 2, 2024

Preview Deployment
941d46136a2d18f47a3bd70db94ddb3cc25e8941

@0x4007
Copy link
Member

0x4007 commented Apr 2, 2024

Something I just realized but if its posting both deploy styles thats not preferred. We should clobber the old style with the new style in this repository. It also would be preferred to match the style because its intended to be easiest to match the shorthand commit that is present on the GitHub UI.

@gentlementlegen
Copy link
Member Author

@pavlovcik Sure please let me know what you have in mind. I did it inside a table because on large screens for me it looks like
image
which is thought is a bit odd.

@0x4007
Copy link
Member

0x4007 commented Apr 2, 2024

It is intended to be visually as close as possible to the current GitHub hash values. This is so that it's most efficient to match them when reviewing and testing. The only problem is that if we post new comments every time there is a significant amount of additional margin and padding which makes the pull request conversation length extremely long.

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.

Custom query params for test deployments
3 participants