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

Solution #41

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Solution #41

wants to merge 3 commits into from

Conversation

L1mbxbx
Copy link

@L1mbxbx L1mbxbx commented Dec 9, 2024

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


It looks like you're almost there! 🚀 However, there are a couple of critical issues that need to be addressed before we can approve your pull request:

  1. Environment Variables: The task requires setting environment variables in the pod manifest, but currently, they are missing. Please add the necessary environment variables to meet the task requirements.

  2. INSTRUCTION.md File: This file is required as part of the task but is currently missing. Please create and include this file with the necessary instructions.

Once these issues are resolved, your solution will meet all the core requirements. Keep up the great work, and don't hesitate to reach out if you need further assistance! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

namespace: todoapp
spec:
selector:
app: todolist

Choose a reason for hiding this comment

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

The selector label app: todolist must match the labels of the pods you want this service to target. Ensure that the pods in the todoapp namespace have the label app: todolist. If the pods have a different label, update this selector accordingly.

spec:
type: NodePort
selector:
app: todolist

Choose a reason for hiding this comment

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

The selector label app: todolist must match the labels of the pods you want this service to target. Ensure that the pods in the todoapp namespace have the label app: todolist. If the pods have a different label, update this selector accordingly.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job on addressing the previous issues! 🎉 Your Kubernetes configurations for the todoapp namespace are mostly on point. Here are a few things to keep in mind:

  • Environment Variables: You've successfully added the APP_ENV environment variable to the pod manifest, which is a crucial step for meeting the task requirements. Well done!

  • Service Configurations: Both the clusterIp.yml and nodeport.yml files are correctly set up for their respective service types. Just ensure that the todolist app label is correctly applied to the pods and that the NodePort 30007 is open and accessible if you plan to access the service externally.

  • Pod Configurations: It's great to see the inclusion of liveness and readiness probes for application health. However, remember to add the missing environment variable setup in the second Pod configuration to maintain consistency.

  • Documentation: The INSTRUCTION.md file is still missing. This file is essential for providing instructions on testing the application using the ClusterIP service, the port-forward command, and accessing the app via a NodePort service. Please make sure to add it.

Your efforts are commendable, and this decision is made to help you move forward with your learning journey. Please review the comments and address the issues to enhance your code. Keep up the good work and continue improving! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

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