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

Upgrade tests #1327

Merged
merged 1 commit into from
May 26, 2021
Merged

Upgrade tests #1327

merged 1 commit into from
May 26, 2021

Conversation

fao89
Copy link
Member

@fao89 fao89 commented May 14, 2021

[noissue]

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@pulpbot
Copy link
Member

pulpbot commented May 14, 2021

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

@fao89 fao89 force-pushed the upgrade_tests branch 8 times, most recently from fad31c6 to fb28495 Compare May 14, 2021 21:32
@fao89 fao89 marked this pull request as ready for review May 14, 2021 21:50
Comment on lines +164 to +166
echo "FROM_PULP_FILE_BRANCH=${{ matrix.FROM_PULP_FILE_BRANCH }}" >> $GITHUB_ENV
echo "FROM_PULPCORE_BRANCH=${{ matrix.FROM_PULPCORE_BRANCH }}" >> $GITHUB_ENV
Copy link
Member Author

Choose a reason for hiding this comment

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

cp -R /tmp/.github .
cp -R /tmp/.ci .
# Pin deps
sed -i "s/~/=/g" requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause deps to not upgrade when the upgrade occurs? I'm not sure so I wanted to ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

we pin deps at CI time only for previous releases,
I did it for dealing with that django issue we had

Copy link
Member

Choose a reason for hiding this comment

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

I see. So 3.11.2 will have that resolved (coming out in a few days) do you think we should merge this for now and undo it later or wait? I'm ok with either. If we're merging now how will we remember to remove this after 3.11.2 is out?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok with both options

@@ -94,6 +107,9 @@ fi
cd pulp-cli
pip install -e .
pulp config create --base-url http://pulp --location tests/settings.toml
sed -i "s/true/false/g" tests/settings.toml
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious what does this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I use the CLI:

pulp status  
pulp file content list

for showing plugin versions, so we can be sure the upgrade really happened, and for display it has content.
When I used that it tried https and broke!

Maybe we can have a better way to handle it, I don't know pulp-cli well

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah pulp-cli uses https as the default, so you're saying this is changing pulp-cli to use http instead of https.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep!

@@ -0,0 +1,136 @@
# coding=utf-8
Copy link
Member

Choose a reason for hiding this comment

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

These are no longer needed with python3

Copy link
Member Author

Choose a reason for hiding this comment

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

copy/paste issue

"""Create class-wide variables."""
cls.cfg = config.get_config()

client = gen_file_client()
Copy link
Member

Choose a reason for hiding this comment

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

Nice bindings tests 👍

@@ -0,0 +1,124 @@
# coding=utf-8
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member Author

Choose a reason for hiding this comment

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

copy/paste issue

@fao89 fao89 force-pushed the upgrade_tests branch 2 times, most recently from e0c0f56 to 0b99fd7 Compare May 19, 2021 14:31
@fao89 fao89 mentioned this pull request May 19, 2021
@fao89 fao89 force-pushed the upgrade_tests branch 2 times, most recently from a26f180 to 27866a7 Compare May 20, 2021 13:18
@fao89 fao89 requested a review from bmbouter May 20, 2021 14:33
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @fao89 !

[noissue]
@fao89
Copy link
Member Author

fao89 commented May 26, 2021

I believe this can be merged now

@bmbouter bmbouter merged commit 23942e4 into pulp:master May 26, 2021
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.

4 participants