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

chore: update mediacloud url to civicsignal url #2

Closed
wants to merge 1 commit into from

Conversation

VinneyJ
Copy link

@VinneyJ VinneyJ commented Jun 19, 2024

Updated the base api URLs from https://api.mediacloud.org/ to https://app.civicsignal.africa:8082/ in api_2_0_spec documention.

@@ -255,7 +255,7 @@ None.

Fetching information on The New York Times

URL: https://api.mediacloud.org/api/v2/media/single/1
URL: https://app.civicsignal.africa:8082/api/v2/media/single/1
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we removing all ports?

Copy link
Author

Choose a reason for hiding this comment

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

@thepsalmist mentioned that the ports are exposed to the frontend apps. @kilemensi

Copy link
Member

Choose a reason for hiding this comment

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

Cool @VinneyJ ... my question is do we want to do this moving forward or not?

Copy link
Author

Choose a reason for hiding this comment

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

No, @kilemensi, this is not the ideal approach. I recommend changing the port exposure. I will discuss this further with @thepsalmist to see how we can approach removing them.

Copy link

@kelvinkipruto kelvinkipruto left a comment

Choose a reason for hiding this comment

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

@VinneyJ LGTM once the exposed port issue is addressed.

@DavidTheProgrammer
Copy link

DavidTheProgrammer commented Jun 21, 2024

@VinneyJ LGTM once the exposed port issue is addressed.

@VinneyJ same here.

@VinneyJ
Copy link
Author

VinneyJ commented Jul 15, 2024

Hello @kilemensi / @thepsalmist since I cloned the repo again let's merge this then I bring in the new changes. What do you say?

@kilemensi
Copy link
Member

It's one file @VinneyJ, isn't it easier to copy it to the new repo and create a new PR?

@VinneyJ
Copy link
Author

VinneyJ commented Jul 15, 2024

Yes @kilemensi that can be another approach.

@DavidTheProgrammer
Copy link

is this PR still valid seeing as we have the updated one? @VinneyJ

@kilemensi
Copy link
Member

Nah, this should automatically be closed once #4 is merged.

@VinneyJ VinneyJ closed this Jul 19, 2024
@VinneyJ VinneyJ deleted the improve-api-2.0-readme branch July 19, 2024 09:18
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.

4 participants