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

Update Map of Pi Repo with Changes Introduced on PCT-managed repo #13

Merged
merged 51 commits into from
Oct 1, 2024

Conversation

tate1650
Copy link
Collaborator

No description provided.

Pi App Engine CI Bot and others added 30 commits August 30, 2024 09:40
Merge GitHub repo code into `main`

See merge request pi-network/apps/mapofpi/mapofpi!1
@tate1650
Copy link
Collaborator Author

@swoocn Feel free to review, raise concerns, and/or approve/merge when you can.

@swoocn
Copy link
Member

swoocn commented Sep 27, 2024

Overall, your changes LGTM, @tate1650! 👍
As long as the app is stable and works for you in the containerized environment, I'm good with it too. That said, it would be helpful if we could run the container ourselves in a local environment to validate your changes. Perhaps we could also consider building a CI/CD setup to automate the workflow, similar to what we have in our Map of Pi repos..?

In the meantime, please address the minor PR comments when you have a chance, and I'll be happy to give you my full approval.
Thanks for all your hard work!

@tate1650
Copy link
Collaborator Author

tate1650 commented Oct 1, 2024

Hey Danny,

Went ahead and introduced the changes you suggested (minus the FE logger rewrites, mostly because I'm short on time). Lemme know what you think.

Re: the CI/CD workflow for a local environment, that probably could be done, but feels a little like overkill to me. Ultimately you'd just need the containers running and communicating with each other on a local machine, so I think all that would really need to be done is set up the app to plug into a sandbox SDK environment (which, correct me if I'm wrong, is already done), and then expose ports via the docker-compose file so that when you're running the containers all together, they can 1. communicate with each other and 2. easily be reached from the host machine (i.e. the one connecting to sandbox.minepi.com).

If you felt so inclined, I don't think you'd even need the reverse-proxy service to accomplish this. This is basically what I'm doing locally (mostly because I was having issues getting the reverse-proxy service to work at the beginning of working on this project and was looking for a method I was more familiar with in the interest of time). If this isn't very clear, or you need my help getting this setup, just lemme know.

Copy link
Member

@swoocn swoocn left a comment

Choose a reason for hiding this comment

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

LGTM @tate1650!
May potentially run into merge conflicts w/ #14, in which case we will need to address. For this reason, and just in case, I would be more comfortable if we merge this PR last.

@tate1650
Copy link
Collaborator Author

tate1650 commented Oct 1, 2024

Alright, works for me!

@tate1650
Copy link
Collaborator Author

tate1650 commented Oct 1, 2024

Looks like we're conflict-free! Merging this one too now.

@tate1650 tate1650 merged commit fd35230 into map-of-pi:dev Oct 1, 2024
@tate1650 tate1650 deleted the update-map-of-pi-repo branch October 1, 2024 02:57
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.

2 participants