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

Report timeout fix #60

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Report timeout fix #60

merged 2 commits into from
Oct 26, 2018

Conversation

baev
Copy link
Member

@baev baev commented Oct 22, 2018

fixes #59
by @Coder1304

*/
@Parameter(property = "allure.serve.port", defaultValue = "0")
protected Integer servePort;
@Parameter(property = "allure.report.timeout", defaultValue = "60")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Parameter(property = "allure.report.timeout", defaultValue = "60")
@Parameter(property = "allure.report.timeout", defaultValue = "600")

Can I suggest a longer default value? 1 minute is not enough for big reports on Windows machines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's to short, that's why this pull was created. This value will be configurable from now, so you would be able to set it to any value you like. 60sec worked here for a long time and from all allure-maven users just two of us starts to complains about that. @baev which default value would be sufficient in your opinion?

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 one minute timeout by default

* Serve timeout parameter in seconds.
*/
@Parameter(property = "allure.serve.timeout", defaultValue = "3600")
private String serveTimeout;
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 don't like this to be a string as well as new introduced report timeout to be a string

Copy link
Contributor

Choose a reason for hiding this comment

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

@baev I send the patch on [email protected] with gh-59-fix-proposition in email subject.

Copy link
Contributor

@Coder1304 Coder1304 Oct 24, 2018

Choose a reason for hiding this comment

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

@baev Could you apply this patch on top of gh-59-fix-proposition and push it? Patch contain change from String to int for timeouts.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Coder1304 still got no email from you, could you please resend it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@baev I've send it again to [email protected] with gh-59-fix-proposition in the subject. And as a reply for the first mail I send you.

@baev baev force-pushed the gh-59-fix-proposition branch from fb8bbcc to e8e10de Compare October 25, 2018 11:43
@baev
Copy link
Member Author

baev commented Oct 25, 2018

Looks good to me.

In order to merge this:

@Coder1304
Copy link
Contributor

Coder1304 commented Oct 25, 2018

@baev I realized that I forgot to change private data. Could you change my private data to this:

removed

I would be more than happy if do this.

@baev baev force-pushed the gh-59-fix-proposition branch from 4a916c5 to 49470b6 Compare October 25, 2018 17:18
@CLAassistant
Copy link

CLAassistant commented Oct 25, 2018

CLA assistant check
All committers have signed the CLA.

@baev
Copy link
Member Author

baev commented Oct 25, 2018

@Coder1304 done, now all we need is CLA

@Coder1304
Copy link
Contributor

@baev Thanks. CLA done.

@baev baev merged commit d91367b into master Oct 26, 2018
@baev baev deleted the gh-59-fix-proposition branch October 26, 2018 12:03
@Coder1304
Copy link
Contributor

@baev I have few questions:

  1. When do you plan to release this change?
  2. If I would like to contribute something to this repo again the process will be the same (create issue/improvement, sending patches to the mail and hoping someone will create pull request )?
  3. Commit on master branch does not contain information who is the author, why is that?

eroshenkoam pushed a commit that referenced this pull request Oct 29, 2018
@eroshenkoam
Copy link
Member

@Coder1304

  1. It will be ready tomorrow)
  2. Just create PR in this repo)
  3. Fixed: https://github.com/allure-framework/allure-maven/commits/master

@Coder1304
Copy link
Contributor

@eroshenkoam Could you change my data to:
--author="Coder1304 <[email protected]>"

eroshenkoam pushed a commit that referenced this pull request Oct 29, 2018
@eroshenkoam
Copy link
Member

done) ty for pr 👍

@Coder1304
Copy link
Contributor

I have to prepare the change first. Thanks.

@Coder1304
Copy link
Contributor

Coder1304 commented Oct 29, 2018

@eroshenkoam
I am getting problem to push anything to you repo.

$ git push -v origin HEAD:some_branch_here
Pushing to [email protected]:allure-framework/allure-maven.git
ERROR: Permission to allure-framework/allure-maven.git denied to Coder1304.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
$ git remote -v
origin	[email protected]:allure-framework/allure-maven.git (fetch)
origin	[email protected]:allure-framework/allure-maven.git (push)

Tried with ssh and https, same result.

@baev
Copy link
Member Author

baev commented Oct 29, 2018

@Coder1304 see https://help.github.com/articles/creating-a-pull-request/

If you don't have write access to the repository where you'd like to create a pull request, you must create a fork, or copy, of the repository first. For more information, see "Creating a pull request from a fork" and "About forks."

@Coder1304
Copy link
Contributor

@baev Thanks a lot for this link.

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.

To short timeout for generateReport method.
5 participants