Skip to content

chore: root module improvements, prepare repo to make public#12

Merged
jorgemoralespou merged 1 commit intomainfrom
root-module-improvements
Jun 4, 2025
Merged

chore: root module improvements, prepare repo to make public#12
jorgemoralespou merged 1 commit intomainfrom
root-module-improvements

Conversation

@billkable
Copy link
Copy Markdown
Collaborator

@billkable billkable commented May 28, 2025

@billkable billkable requested a review from jorgemoralespou May 28, 2025 14:08
@billkable billkable force-pushed the root-module-improvements branch from 5cdb712 to 277faa7 Compare May 28, 2025 18:00
Copy link
Copy Markdown
Contributor

@jorgemoralespou jorgemoralespou left a comment

Choose a reason for hiding this comment

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

Left some comments

CONTRIBUTING.md Outdated

If your primary objective is to use Educates, check out the [Educates user documentation](https://docs.educates.dev/).

If you want to contribute changes to Educates, check out the [Educates developer documentation](developer-docs/README.md).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This link will need to be updated to point to educates-training-platform repo

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CONTRIBUTING.md Outdated

### Pull Request Checklist

1. Ensure that you have not included any personal or company copyright notices in any changes. This project relies on a single copyright assignment in the [NOTICE](NOTICE) file with attribution to "The Educates Authors".
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file does not exist. Does it exist in the original repo? Should this be updated there as well?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jorgemoralespou the contribution guide is changed to point to Educates Training Portal repo, will file issue there.

If you have some common configurations you want to share,
[submit a PR](#release-a-module). We'll review and potentially add it.

## Gitops configuration
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this rely on the other module to be released?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jorgemoralespou no. if you read the 2nd section, you'll see implicit TODO.

# Set the following variables in your shell environment
export aws_region="AWS_REGION"
export aws_account_id="AWS_ACCOUNT_ID"
export iams_principal="IAM_PRINCIPAL"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at this file, I wouldn't know what to put as IAM_PRINCIPAL, and couldn't figure it out looking at the rest of the files in here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jorgemoralespou documented in README Configuration Examples

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Latest Educates released version to date is 3.3.2

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

# Configuration for the Educates Installer
##
variable "deploy_educates" {
description = "Whether to enable and install Educates platform"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Description does not match name. I would change it to: "Whether to deploy/install Educates platform onto the cluster"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as before

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

# Configuration for the Educates Installer
##
variable "deploy_educates" {
description = "Whether to enable and install Educates platform"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as before

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this script file? Since it's not easy to grok, I would add some comment explaining the file, and potentially a README.md somewhere at the root-modules folder level

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jorgemoralespou script and env.template files removed - see readme configuration section where configuration parameters are documented.

@billkable
Copy link
Copy Markdown
Collaborator Author

Per discussion with @jorgemoralespou, will remove the template/script, but will document the placeholder variables.

@billkable billkable force-pushed the root-module-improvements branch 2 times, most recently from 5e1ec8c to fd2951d Compare June 3, 2025 17:19
Copy link
Copy Markdown
Contributor

@jorgemoralespou jorgemoralespou left a comment

Choose a reason for hiding this comment

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

There's still 3.2.2 references instead of 3.3.2, but I'm ok if you want to merge like this, otherwise do a global find/replace of 3.2.2 with 3.3.2 and we should be ok to go.

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still 3.2.2

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still 3.2.2

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still 3.2.2

kubernetes_version = "1.32"
cluster_name = "{{cluster_name}}"
deploy_educates = true
educates_version = "3.2.2"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still 3.2.2

Do a global find/replace of 3.2.2 with 3.3.2 and we should be ok to go.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@billkable billkable force-pushed the root-module-improvements branch from 2aa60d9 to 903c452 Compare June 4, 2025 14:05
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 4, 2025

Release Plan

Module Release Type Latest Version New Version
platform/educates patch v1.0.1 v1.0.2
🚫 Wiki Check: Generation is disabled.

Changelog

platform/educates/v1.0.2 (2025-06-04)

  • 🔀PR #12 - chore: root module improvements, prepare repo to make public

Powered by techpivot/terraform-module-releaser

@billkable
Copy link
Copy Markdown
Collaborator Author

@jorgemoralespou I think we are good. the topic branch is fixed up, should be ready to merge.

@jorgemoralespou jorgemoralespou merged commit 037b9c2 into main Jun 4, 2025
1 check passed
@jorgemoralespou jorgemoralespou deleted the root-module-improvements branch June 4, 2025 14:08
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 4, 2025

🚀 Terraform Module Releases

The following Terraform modules have been released:

Powered by techpivot/terraform-module-releaser

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