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

Adds issue templates for other projects #829

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

gurkanindibay
Copy link
Contributor

No description provided.

Copy link
Member

@hanefi hanefi left a comment

Choose a reason for hiding this comment

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

I guess you copied the checlist for Citus and assumed that there are not many changes for the other projects. Please go over them one by one and make sure they have necessary changes for each project.

@gurkanindibay gurkanindibay requested a review from hanefi October 8, 2021 12:57
@@ -66,8 +66,7 @@ jobs:
--secret_key "${PACKAGING_SECRET_KEY}" \
--passphrase "${PACKAGING_PASSPHRASE}" \
--output_dir "$(pwd)/packages/" \
--input_files_dir "$(pwd)/packaging" \
--output_validation
--input_files_dir "$(pwd)/packaging"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed output validation since an erronous commit made nightlies failed so the tasks failed. I think output valdation should not be null in nightlies since some warning in documentation make it failed

Copy link
Member

@hanefi hanefi left a comment

Choose a reason for hiding this comment

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

Please address my comments for enterprise checklist for all applicable checklists.

Please do the following:

  • Remove $VERSION, $PROJECT, and $REPO references. If you decide to update the remove the information on these variables from the checklist, you should remove all the references as well.
  • Update the formats of all the links. [text to be shown](http://example.com) is usually nicer to read.
  • Move the subset of your changes that disable output validation for nightlies in a separate PR. That change is not directly related to this PR.

@gurkanindibay
Copy link
Contributor Author

gurkanindibay commented Nov 25, 2021

Please address my comments for enterprise checklist for all applicable checklists.

Please do the following:

  • Remove $VERSION, $PROJECT, and $REPO references. If you decide to update the remove the information on these variables from the checklist, you should remove all the references as well.
  • Update the formats of all the links. [text to be shown](http://example.com) is usually nicer to read.
  • Move the subset of your changes that disable output validation for nightlies in a separate PR. That change is not directly related to this PR.

Removed $PROJECT and $REPO references but I think we shouldn't remove the $VERSION reference since this parameter is changing in each release

@hanefi
Copy link
Member

hanefi commented Nov 27, 2021

@gurkanindibay I did not re-review this PR, as some of the tests are pending. FYI.

@gurkanindibay
Copy link
Contributor Author

gurkanindibay commented Nov 29, 2021

@gurkanindibay I did not re-review this PR, as some of the tests are pending. FYI.

It was task list and I intentionally left unchecked since Version reference is required I think
Now checked this task for you to be be able ro review
Thanks.

@gurkanindibay gurkanindibay requested a review from hanefi November 29, 2021 04:35
Copy link
Member

@hanefi hanefi left a comment

Choose a reason for hiding this comment

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

I left reviews for enterprise checklist only. Most of the changes apply for all the templates.

Also there are ~10 lines with spacing issues. Please use a linter/autoformatter that can fix markdown files and fix those as well.

# Update OS Packages
## Debian and RedHat
- Change your directory to `packaging` repository directory & checkout `all-enterprise` branch.
- [ ] Run [this pipeline](https://github.com/citusdata/packaging/actions/workflows/update_package_properties.yml) using branch name as `all-enterprise`. Input tag name and if version is fancy, input the fancy version number. Other parameters could be kept as is if you want.
Copy link
Member

Choose a reason for hiding this comment

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

IMO, It is a nice idea to have names of the pipelines in human readable format.

Suggested change
- [ ] Run [this pipeline](https://github.com/citusdata/packaging/actions/workflows/update_package_properties.yml) using branch name as `all-enterprise`. Input tag name and if version is fancy, input the fancy version number. Other parameters could be kept as is if you want.
- [ ] Run [Update Package Properties pipeline](https://github.com/citusdata/packaging/actions/workflows/update_package_properties.yml) using branch name as `all-enterprise`. Input tag name and if version is fancy, input the fancy version number. Other parameters could be kept as is if you want.

Comment on lines +14 to +22
- Then check the following (needed for both debian & redhat):
- [ ] Updated `pkglatest` variable in the `pkgvars` file to `$VERSION.citus-1`
- Then check the following (needed for debian):
- [ ] A new entry (`$VERSION.citus-1`, `stable`) is added to the `debian/changelog` file
- Then check the following (needed for redhat):
- [ ] `citus.spec` file is updated:
- [ ] `Version:` field
- [ ] `Source0:` field
- [ ] A new entry (`$VERSION.citus-1`) in the `%changelog` section
Copy link
Member

Choose a reason for hiding this comment

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

Does it really make sense to have different sections here?

Suggested change
- Then check the following (needed for both debian & redhat):
- [ ] Updated `pkglatest` variable in the `pkgvars` file to `$VERSION.citus-1`
- Then check the following (needed for debian):
- [ ] A new entry (`$VERSION.citus-1`, `stable`) is added to the `debian/changelog` file
- Then check the following (needed for redhat):
- [ ] `citus.spec` file is updated:
- [ ] `Version:` field
- [ ] `Source0:` field
- [ ] A new entry (`$VERSION.citus-1`) in the `%changelog` section
- Then check the following file changes:
- [ ] Updated `pkglatest` variable in the `pkgvars` file to `$VERSION.citus-1`
- [ ] A new entry (`$VERSION.citus-1`, `stable`) is added to the `debian/changelog` file
- [ ] `citus.spec` file is updated:
- [ ] `Version:` field
- [ ] `Source0:` field
- [ ] A new entry (`$VERSION.citus-1`) in the `%changelog` section

- [ ] Get changes reviewed; merge the PR
- [ ] Ensure Github Actions builds completed successfully and package count for each os is as below table and packages in postgres versions is compliant with `postgres-matrix.yml` in the `all-project` branch

https://github.com/citusdata/tools/blob/be12af3b8f435d17a52e607c666f6b15379f5970/packaging_automation/tests/test_citus_package.py#L21-L33
Copy link
Member

Choose a reason for hiding this comment

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

What about having a human readable link with some explanation that will make it easier for the reader to understand why this is here?

If there is no reason to have it here, you can drop this line as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Ensure Github Actions builds completed successfully and package count for each os is as below table and packages in postgres versions is compliant with postgres-matrix.yml in the all-project branch

This is the description but it seems that it is not as definitive as I intended
What's your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

There are some issues with the current link:

  1. The link contains a sha, and that means the link needs to be updated when new distros are introduced in the file or the package counts for a distro changed for some reason.
  2. The tests are supposed to check the number of generated files, right? It does not make sense for an issue template to have an item that wants the user to read some piece of code, and use the values in there to check if the count of generated packages are correct.

My suggestion is to either find a better and easier alternative to this, or drop the link completely.

A possible alternative is to check the total number of packages generated, and dive deeper if the total does not match.

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