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

✅ remove ignite/starport #207

Merged
merged 37 commits into from
Aug 4, 2022
Merged

✅ remove ignite/starport #207

merged 37 commits into from
Aug 4, 2022

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 18, 2022

The ignite cli has a terrible track record of keeping dependencies like cosmoscmd up to date.

For security reasons, we should remove any depencency that lives in its repository.

For maintainability reasons, same thing.

New plan is to migrate over the cli tests too. I think it's possible that they'll help me to identify the issue.

@faddat faddat requested review from joeabbey and giansalex May 18, 2022 23:17
@faddat faddat mentioned this pull request May 18, 2022
app/app.go Show resolved Hide resolved
@faddat faddat marked this pull request as draft May 20, 2022 07:24
@faddat faddat changed the title remove ignite/starport DNM (but please review): remove ignite/starport Jun 2, 2022
@faddat faddat marked this pull request as ready for review June 2, 2022 10:21
@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

Note:

I can't figure out why the docker container keeps failing, causing the CI tests for gov (which we want to keep) are failing.

Do we happen to use the ignite cli at all in the flow?

@sascha1337
Copy link

Note:

I can't figure out why the docker container keeps failing, causing the CI tests for gov (which we want to keep) are failing.

Do we happen to use the ignite cli at all in the flow?

Without testing it on my end, i guess it's failing due to some incorrect working directory at the runtime execution - i did a quick scroll thru the workflow log ~ and i think i saw that the chronologic order of the gh actions might be incorrect.

The logfile tells us that the docker compose up command is running against the wall - this action is being executed AFTER cloning cw-unity-prop repo, which does not have a docker compose file. That should be ran on the juno(d) workdir, not cw unity prop.

Maybe.

@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

Thank you very much for your review, that is actually a bug that I introduced while testing CI, and I will revert it shortly.

Sorry about that!

@the-frey
Copy link
Collaborator

the-frey commented Jul 21, 2022

Ok:

  • Main needs merging in
  • May as well change v8 -> v9 since we're now working against v9 candidate

And then I think I'm happy with merging this and moving it forward for testing.

@sascha1337
Copy link

Gonna do that now out of curiosity

@faddat
Copy link
Contributor Author

faddat commented Jul 26, 2022

on it.

@faddat
Copy link
Contributor Author

faddat commented Jul 26, 2022

@the-frey hey, main is merged in already afaik

@sascha1337 if you're working on juno routinely, please feel free / encouraged to make PR's into branches like these, basically to keep up velocity.

the-frey
the-frey previously approved these changes Jul 26, 2022
Copy link
Collaborator

@the-frey the-frey left a comment

Choose a reason for hiding this comment

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

LGTM. Will also tag this with a pre-release once it's on main so I can run a bunch of local checks against the binary etc

@sascha1337
Copy link

@the-frey hey, main is merged in already afaik

@sascha1337 if you're working on juno routinely, please feel free / encouraged to make PR's into branches like these, basically to keep up velocity.

I've literally seen that on the iOS app I had to turn on notifications manually, now I see what's going on when watching repos :) automatically being in the feedback loop

Plus whitelisting GitHub notifications when turning on do not disturb / work modus across devices, small thing but literally game changer 😁 LGTMFAM

@the-frey
Copy link
Collaborator

the-frey commented Aug 1, 2022

@faddat Is this now updated and R4M? I've already approved but I think GH is prompting me to smash that tick button again.

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

Yep, this is ready to rock.

@the-frey
Copy link
Collaborator

the-frey commented Aug 3, 2022

@joeabbey would you mind giving this a review dude?

@the-frey the-frey self-requested a review August 3, 2022 10:12
cmd/junod/root.go Outdated Show resolved Hide resolved
cmd/junod/root.go Outdated Show resolved Hide resolved
@the-frey
Copy link
Collaborator

the-frey commented Aug 3, 2022

Spotted maybe 1 thing that might be worth renaming, but GTM apart from that (and apols if I've mis-remembered where the app info via CLI gets logged from - the Stargate Cosmos App thing rather than a configurable app name always annoyed me).

@faddat
Copy link
Contributor Author

faddat commented Aug 3, 2022

they're both worth changing. thanks.

@the-frey
Copy link
Collaborator

the-frey commented Aug 4, 2022

Have changed. If CI passes then it's GTM

@the-frey the-frey self-requested a review August 4, 2022 13:48
Copy link
Collaborator

@the-frey the-frey left a comment

Choose a reason for hiding this comment

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

LGTM

@faddat faddat merged commit 34f8987 into main Aug 4, 2022
@faddat faddat mentioned this pull request Aug 4, 2022
6 tasks
@the-frey the-frey deleted the no-starport2 branch September 9, 2022 15:36
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.

None yet

6 participants