search

pokt-foundation / grove-portal

pocket-portal.vercel.app
MIT License
6 stars 6 forks source link

[BUG REPORT] Quick Retro During #672 #673

Closed Olshansk closed 1 month ago

Olshansk commented 1 month ago

Super quick retro while working on #672 and looking for feedback from @commoddity:

  1. Can we remove the git hooks from grove-portal? They really get in the way and not reliable I was using --no-verify myself.
  2. Can we remove this requirement from CI [1]. Again, it was a ❌ that I ignored.
  3. Can we remove all plan types that aren't being used? It will eliminate code paths we may have overlooked 
    export enum PayPlanType {
  Enterprise = 'ENTERPRISE',
  FreetierV0 = 'FREETIER_V0',
  PayAsYouGoV0 = 'PAY_AS_YOU_GO_V0',
  PlanFree = 'PLAN_FREE',
  PlanUnlimited = 'PLAN_UNLIMITED',
  TestPlan_10K = 'TEST_PLAN_10K',
  TestPlan_90K = 'TEST_PLAN_90K',
  TestPlanV0 = 'TEST_PLAN_V0'
}
  4. Need to update documentation that deployment flow is:

[1] https://github.com/pokt-foundation/grove-portal/pull/671#issuecomment-2344993769

commoddity commented 1 month ago

Super quick retro while working on #672 and looking for feedback from @commoddity:

  1. Can we remove the git hooks from grove-portal? They really get in the way and not reliable I was using --no-verify myself.

TBH I don't have a strong opinion on this.

One thought is this: they were put there for a reason by a frontend team that very much knew what they were doing.

Given that we don't have a single dedicated frontend engineer on the team and are trying to maintain a codebase built by other people, does it not make sense to keep those in place to help ensure any changes we make are passing the tests the former front end team put in place?

I know this PR was done in a hurry to fix an urgent bug but I imagine future changes to the UI will be done in a more relaxed way so we can take the time to work through any issues with the checks in the case that they are not passing.

  1. Can we remove this requirement from CI [1]. Again, it was a ❌ that I ignored.

Same answer as above, although I'd add that this error has been very annoying to me in the past as well. I'm not super opposed to removing it but I do have a slight hesitation to remove CI checks that were put there for reasons we don't know, especially given the reasoning in my answer to [1].

@fredteumer is this related to the issue you mentioned where the person who merges the PR has to be an owner on Vercel:

0s
Run vercel pull --yes --environment=preview --token=***
Vercel CLI 37.4.2
Retrieving project…
Error: Could not retrieve Project Settings. To link your Project, remove the `.vercel` directory and deploy again.
Learn More: https://vercel.link/cannot-load-project-settings
Error: Process completed with exit code [1](https://github.com/pokt-foundation/grove-portal/actions/runs/10821569024/job/30023836574#step:8:1).
  1. Can we remove all plan types that aren't being used? It will eliminate code paths we may have overlooked 
    export enum PayPlanType {
 Enterprise = 'ENTERPRISE',
 FreetierV0 = 'FREETIER_V0',
 PayAsYouGoV0 = 'PAY_AS_YOU_GO_V0',
 PlanFree = 'PLAN_FREE',
 PlanUnlimited = 'PLAN_UNLIMITED',
 TestPlan_10K = 'TEST_PLAN_10K',
 TestPlan_90K = 'TEST_PLAN_90K',
 TestPlanV0 = 'TEST_PLAN_V0'
}

Sure. I can open a PR to do that.

  1. Need to update documentation that deployment flow is:

Same. I'll do that.

fredteumer commented 1 month ago

We don't have frontend developers on the team anymore so I understand the concerns on removing the pre-commit hooks from the CI. I do find them very clunky -- if we're going to be the maintainers going forward, I think we should either take the time to understand and remedy them or remove them.

I don't have a hard and fast recommendation here since our touching of the frontend should be limited for the foreseeable future (knock on wood), but my gut tells me it's probably better to understand them rather than remove them. Open to @Olshansk 's thoughts though.

commoddity commented 1 month ago

We don't have frontend developers on the team anymore so I understand the concerns on removing the pre-commit hooks from the CI. I do find them very clunky -- if we're going to be the maintainers going forward, I think we should either take the time to understand and remedy them or remove them.

I don't have a hard and fast recommendation here since our touching of the frontend should be limited for the foreseeable future (knock on wood), but my gut tells me it's probably better to understand them rather than remove them. Open to @Olshansk 's thoughts though.

Thanks for the reply @fredteumer .

My suggestion is we don't touch the pre-commit/push checks. For me at least they seem to pass reliably.

However for the CI checks, I am removing the Post test coverage job as it seems to fail 100% of the time.

My only remaining question is regarding the Pull Vercel Environment Information CI job. Does this test only fail because the person who merged the PR is not the Vercel owner, or for some other reason?

If it's the former let's just update the README.md to ensure we know the reason. If it's the latter then... I guess let's discuss.

commoddity commented 1 month ago

@Olshansk @fredteumer PR #675 will close this issue.

However, before merging 675, these two PRs must be merged:

commoddity commented 1 month ago

@ols

@Olshansk @fredteumer PR #675 will close this issue.

However, before merging 675, these two PRs must be merged:

@Olshansk @fredteumer Thanks for the reviews, here are the app of apps PRs for PUB: STAGING - https://github.com/pokt-foundation/appOfApps/pull/2138 PRODUCTION - https://github.com/pokt-foundation/appOfApps/pull/2139

Order should be: PUB staging -> UI staging -> PUB prod - UI prod

fredteumer commented 1 month ago

My only remaining question is regarding the Pull Vercel Environment Information CI job. Does this test only fail because the person who merged the PR is not the Vercel owner, or for some other reason?

If it's the former let's just update the README.md to ensure we know the reason. If it's the latter then... I guess let's discuss.

Looking at past PRs (even ones I have done) it appears this always fails. I would think this indicates that means we don't know the reason it is failing. Can we test to see if following it's instructions by removing the .vercel directory and pushing again / triggering the deployment works as it is expecting? it does appear to be properly noted in the .gitignore file

commoddity commented 1 month ago

My only remaining question is regarding the Pull Vercel Environment Information CI job. Does this test only fail because the person who merged the PR is not the Vercel owner, or for some other reason? If it's the former let's just update the README.md to ensure we know the reason. If it's the latter then... I guess let's discuss.

Looking at past PRs (even ones I have done) it appears this always fails. I would think this indicates that means we don't know the reason it is failing. Can we test to see if following it's instructions by removing the .vercel directory and pushing again / triggering the deployment works as it is expecting? it does appear to be properly noted in the .gitignore file

I've tried that. .vercel is .gitignored so removing it locally should make no difference. Regardless I tried anyway and it had no effect.

fredteumer commented 1 month ago

I think if i update the access token it should work?

https://github.com/orgs/vercel/discussions/3260 https://vercel.com/guides/how-can-i-use-github-actions-with-vercel https://vercel.com/guides/how-do-i-use-a-vercel-api-access-token

fredteumer commented 1 month ago

@commoddity i've updated the VERCEL_TOKEN in this repository. Please lmk if this resolves this issue.