-
Notifications
You must be signed in to change notification settings - Fork 82
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
[UI] Dynamically adding front-matter to documentation pages #379
Conversation
…ricing-list workflow Signed-off-by: Ankita Sahu <[email protected]>
✅ Deploy Preview for bejewelled-pegasus-b0ce81 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Ankita Sahu <[email protected]>
// @lekaf974 |
.github/workflows/pricing-list.yml
Outdated
|
||
- name: Wait for Pricing List workflow to complete | ||
run: | | ||
# You may need to implement a polling mechanism here to check the status of the other workflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consider that the other workflow trigger this one once done ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can have the other workflow trigger this one when it's done, rather than relying on a polling mechanism.
.github/workflows/pricing-list.yml
Outdated
|
||
- name: Pull latest changes | ||
run: | | ||
git pull origin master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the reason to pull master since a previous step is already doing a checkout ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for pulling from master after the checkout step is because the first workflow (which generates the pricing-data.json) pushes changes to the repository, so when the current workflow runs, it needs to pull the updated data to ensure it has the latest pricing-data.json. This ensures the front matter is updated based on the most recent pricing information generated by the other workflow.
But you're right! Since we are performing the checkout after triggering the other workflow, the newly added or modified .json file from the "generate-pricing-list.yml" workflow should already be included when the checkout step is executed.
update_frontmatter.go
Outdated
|
||
func main() { | ||
// Read the JSON file | ||
jsonFile, err := ioutil.ReadFile("pricing_data.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From where the file is coming ?
Could we also use a environment variable instead of hardcoded value for the file name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is added from the "generate-pricing-list.yml" workflow call.
Yes, we can absolutely use an environment variable, I'll add one, Thanks!
update_frontmatter.go
Outdated
|
||
// Extract hash if present | ||
urlHash := "" | ||
for i, part := range urlParts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather put this logic in a separate method with a extractHah
to make code more readable and to remove the comment
update_frontmatter.go
Outdated
} | ||
|
||
// Build the path step by step | ||
for _, part := range urlParts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same buildPath
update_frontmatter.go
Outdated
|
||
if urlHash == "" { | ||
// No hash: add shortcode after the front matter | ||
frontMatterEnd := strings.Index(contentStr, "---") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here having a method with proper name for readability
update_frontmatter.go
Outdated
} | ||
} else { | ||
// Hash present: search for matching heading and add shortcode below | ||
hashPattern := strings.ReplaceAll(urlHash, "-", "[-\\s]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here having a method with proper name for readability
Co-authored-by: Matthieu Evrin <[email protected]> Signed-off-by: Lee Calcote <[email protected]>
Suggested changes accepted. |
Signed-off-by: Ankita Sahu <[email protected]>
update_frontmatter.go
Outdated
return | ||
} | ||
|
||
// Group entries by documentation URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove all the comments since just reading the code is enough to understand what it is doing.
Usually comments are used for developers documentation or if the algorithm is doing something complex or we need to have context to understand which is not the case here.
Otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll get rid of all the comments, Thanks!
I did reviews on go script but could you review the html css part I'm less comfortable on this |
It's coming along! |
@SAHU-01 |
Signed-off-by: Ankita Sahu <[email protected]>
Signed-off-by: Vivek Vishal <[email protected]>
@SAHU-01 Please add it as an agenda item to the meeting minutes. |
Signed-off-by: Ankita Sahu <[email protected]>
…ricing page added Signed-off-by: Ankita Sahu <[email protected]>
Signed-off-by: Ankita Sahu <[email protected]>
adding |
Signed-off-by: Ankita Sahu <[email protected]>
This flowchart explains how the workflows are working: https://playground.meshery.io/extension/meshmap?mode=design&design=cbb68338-be01-4e0c-b833-3b88c024ca61 |
script: | | ||
const result = await github.rest.actions.createWorkflowDispatch({ | ||
owner: 'layer5labs', | ||
repo: 'meshery-extensions-packages', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of running the workflow over here? @vishalvivekm @SAHU-01 @jerensl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow fetches the integration sheet and parses the csv and gives us a json file, additionally, it filters out and returns rows of the sheet in json that we need to map, i.e. if a feature on the sheet has been documented or has been marked to be displayed on pricing page.
@SAHU-01 do you have done a diagram that explain those github workflows dependencies? |
Signed-off-by: Ankita Sahu <[email protected]>
Signed-off-by: Ankita Sahu <[email protected]>
Notes for Reviewers
This PR fixes #89
Signed commits