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

Automate the branch entry to version increment workflow #2602

Merged

Conversation

prudhvigodithi
Copy link
Collaborator

@prudhvigodithi prudhvigodithi commented Sep 9, 2022

Description

Automate the branch entry to version increment workflow

Issues Resolved

Part of: #1375
Solves: #2601

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

* Updated manifests.

Signed-off-by: opensearch-ci-bot <[email protected]>

* Remove 2.2.2 from the check for build as 2.3.0 is about to release

Signed-off-by: Peter Zhu <[email protected]>

* Add common-utils

Signed-off-by: Peter Zhu <[email protected]>

Signed-off-by: opensearch-ci-bot <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: opensearch-ci-bot <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
@prudhvigodithi prudhvigodithi requested a review from a team as a code owner September 9, 2022 21:58
@prudhvigodithi
Copy link
Collaborator Author

prudhvigodithi commented Sep 9, 2022

Test fail for mypy strange this works on my local

pipenv run mypy .
Success: no issues found in 323 source files

@prudhvigodithi prudhvigodithi force-pushed the versionincrement branch 3 times, most recently from a5e569c to 0212aac Compare September 9, 2022 22:48
@peterzhuamazon
Copy link
Member

@prudhvigodithi can you explain what you are trying to do here?
I see you are trying to load a yaml and check the matrix.
While figuring out what is missing and add new entries, but not very clear without the data. Thanks.

@prudhvigodithi
Copy link
Collaborator Author

prudhvigodithi commented Sep 12, 2022

@prudhvigodithi can you explain what you are trying to do here? I see you are trying to load a yaml and check the matrix. While figuring out what is missing and add new entries, but not very clear without the data. Thanks.

So @peterzhuamazon in order to trigger the version-increment workflow, we need the right branch entries, having these branches, for every branch the a job is created and version increment is performed, now for every release one has to raise a PR to add this branch entry.

With this change (part of this PR) this branch entry with be auto updated as part of the manifest workflow [AUTO] Updated input manifests PR, so next time we dont need to manually raise a PR for 2.x, 3.x etc.

@peterzhuamazon
Copy link
Member

I see what you mean so if there is any new branches it will update the versions-increment yml workflow so we dont need to manually raise PR.

Thanks.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #2602 (d3964dc) into main (d502f4a) will decrease coverage by 0.47%.
The diff coverage is 28.12%.

@@             Coverage Diff              @@
##               main    #2602      +/-   ##
============================================
- Coverage     94.41%   93.94%   -0.48%     
  Complexity       28       28              
============================================
  Files           219      219              
  Lines          4462     4494      +32     
  Branches         29       29              
============================================
+ Hits           4213     4222       +9     
- Misses          243      266      +23     
  Partials          6        6              
Impacted Files Coverage Δ
src/manifests_workflow/input_manifests.py 82.97% <28.12%> (-16.11%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@prudhvigodithi prudhvigodithi force-pushed the versionincrement branch 3 times, most recently from e39b644 to 4139168 Compare September 12, 2022 22:59
@prudhvigodithi prudhvigodithi marked this pull request as draft September 12, 2022 23:03
@prudhvigodithi prudhvigodithi marked this pull request as ready for review September 13, 2022 00:27
Signed-off-by: prudhvigodithi <[email protected]>
@prudhvigodithi
Copy link
Collaborator Author

prudhvigodithi commented Sep 13, 2022

Thanks @peterzhuamazon, I have also just removed - '2.3' (commit) from the entry as we are already on 2.4, just an overkill to run again for all 2.3 branches

@dblock
Copy link
Member

dblock commented Sep 13, 2022

I’m late to this PR, but it needs more work even if it seems to work. Please followup / open issues accordingly.

  1. What is the new yaml library business here? It looks like there’s now more than one way to read and write YML, please consolidate into one.
  2. There are no tests that ensure that the correct values were added to the workflow in the various cases of major/minor/etc., just that the add method was called.

@patch("builtins.open", new_callable=mock_open)
@patch("manifests_workflow.input_manifests.InputManifests.add_to_versionincrement_workflow")
def test_add_to_versionincrement_workflow(self, *mocks: Any) -> None:
input_manifests = InputManifests("test")
Copy link
Member

@dblock dblock Sep 13, 2022

Choose a reason for hiding this comment

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

This test doesn’t assert anything.

with open(versionincrement_workflow_file) as f:
data = yaml.load(f)

version_entry = []
Copy link
Member

Choose a reason for hiding this comment

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

This part is not tested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me add this as well, I will raise a new PR

@@ -11,6 +11,8 @@
from abc import abstractmethod
from typing import Dict, List, Type, Union

import ruamel.yaml
Copy link
Member

Choose a reason for hiding this comment

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

The code now reads and writes YML files in two different ways. We need to have one.

@prudhvigodithi
Copy link
Collaborator Author

Hey @dblock for the new YAML library ruamel.yaml I have have added useful information #2602 (comment), please check. I can create an issue to migrate to ruamel.yaml

@prudhvigodithi
Copy link
Collaborator Author

Created a new issue to use ruamel.yaml #2614
@dblock @peterzhuamazon @gaiksaya @bbarani

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.

5 participants